Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #2486: build log for autodeployment is not working #2514

Merged
merged 1 commit into from
Nov 24, 2021

Conversation

Andrewnt219
Copy link
Contributor

@Andrewnt219 Andrewnt219 commented Nov 23, 2021

Issue This PR Addresses

Fixes #2486

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

  • Fix build log doesn't parse the data correctly, so it always returns false (i.e. no on-going build).
  • Change files structure to align with other scripts.

image
image

Test plan

  • Change autodeploymentUrl in public/js/buildl-log/api.js
- const autodeploymentUrl = (path) => `//${location.hostname}:4000/${path}`;
+ const autodeploymentUrl = (path) => `//dev.api.telescope.cdot.systems:4000/${path}`;  
  • Allow all src in src/server.js
-         connectSrc: [
-          "'self'",
-          '*.fontawesome.com',
-          `${host.replace(/(^\w+:|^)\/\//, '')}:4000`,
-          '*.github.com',
-        ],
+ connectSrc: ["'self'", "*"]
  • Go to /src/api/status.
  • Run npm run dev
  • Go to pages/build.html
  • Build log only shows up if there is an on-going build

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@gitpod-io
Copy link

gitpod-io bot commented Nov 23, 2021

@manekenpix manekenpix self-requested a review November 23, 2021 07:21
manekenpix
manekenpix previously approved these changes Nov 23, 2021
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing to see!

const res = await fetch(autodeploymentUrl('status'));
const res = await fetch(autodeploymentUrl('status'), {
headers: {
'Cache-Control': 'no-cache',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to happen on the response vs. request. Do this in the autodeployment server here: https://github.com/Seneca-CDOT/telescope/blob/master/tools/autodeployment/builds.js#L100-L129

You can add this to each of those before you send the result:

res.setHeader('Cache-Control', 'no-cache');

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: this can (and probably should) happen in a separate PR so we don't block this work. I would just remove the bit above to add headers here for now, and do a second PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll make another PR today.

humphd
humphd previously approved these changes Nov 23, 2021
@humphd humphd requested a review from manekenpix November 23, 2021 15:56
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Andrewnt219 nice job, go ahead and merge this yourself

@Andrewnt219 Andrewnt219 merged commit 17d5c15 into Seneca-CDOT:master Nov 24, 2021
@Andrewnt219
Copy link
Contributor Author

Should have done a "squash and merge". "Rebase and merge" took my commit as a title instead of the PR's title.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build log for autodeployment is not working properly
3 participants