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
Remove custom healthcheck of status service #3280
Conversation
4d75d8b
to
35a0b51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is a good idea. If I remember correctly, this change was made because the backend
service is going to disappear once all the microservices are implemented, and we won't be able to get the information we need to display version and commit, that's why status
was selected as the provider of this information. There's a conversation about injecting this information at build time in the CI, but that would make some parts of the hero banner almost undebuggable, so I don't know what the right solution is here.
pnpm-lock.yaml
Outdated
@@ -343,11 +342,10 @@ importers: | |||
'@octokit/plugin-retry': 3.0.9 | |||
'@octokit/plugin-throttling': 3.5.2_@octokit+core@3.5.1 | |||
'@popperjs/core': 2.11.4 | |||
'@senecacdot/satellite': 1.27.0 | |||
'@senecacdot/satellite': link:../../satellite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @DukeManh, this shouldn't be happening, right? Has that .npmrc
change not landed yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like Kevan just picked up the change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kevan-Y, there's a recent change on master
to download published packages from the npm registry versus using local packages.
To pick it up, remove pnpm-lock
and let pnpm generates the file again.
pnpm-lock.yaml
Outdated
@@ -357,7 +355,7 @@ importers: | |||
xterm-addon-web-links: 0.5.1_xterm@4.18.0 | |||
xterm-addon-webgl: 0.11.4_xterm@4.18.0 | |||
devDependencies: | |||
'@senecacdot/eslint-config-telescope': 1.1.0_eslint@7.32.0 | |||
'@senecacdot/eslint-config-telescope': link:../../../tools/eslint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be the same issue that David mentioned above? I'm not familiar with the latest stuff added since Satellite was recently incorporated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seconding @manekenpix's suggestion that we keep the info for now. I didn't properly incorporate the healthcheck when I wrote it awhile back so my fault for this
@humphd , @manekenpix, @HyperTHD How about removing the custom healthcheck in status services, and in our front-end we do the fetch directly there. telescope/src/web/src/components/Banner.tsx Line 145 in d8e1f7d
We can fetch this https://raw.githubusercontent.com/Seneca-CDOT/telescope/master/package.json, which get the data of our package.json. |
d69e90d
to
1a93533
Compare
pnpm-lock.yaml
Outdated
@@ -343,11 +342,10 @@ importers: | |||
'@octokit/plugin-retry': 3.0.9 | |||
'@octokit/plugin-throttling': 3.5.2_@octokit+core@3.5.1 | |||
'@popperjs/core': 2.11.4 | |||
'@senecacdot/satellite': 1.27.0 | |||
'@senecacdot/satellite': link:../../satellite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kevan-Y, there's a recent change on master
to download published packages from the npm registry versus using local packages.
To pick it up, remove pnpm-lock
and let pnpm generates the file again.
1a93533
to
eb0d467
Compare
Add version fetching in front-end
eb0d467
to
5963cb5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Learned more nginx stuff from this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, if I understand everything from this PR, in the case of deploying for production, we would need to generate a config that has the specific SHA for that release.
Very good work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue This PR Addresses
Fixes #3263
Type of Change
Description
Removed custom healthcheck in status service.getPackage()
is not needed here. Because we don't have proper try/catch getPackage + with recent change in Dockerfile with Healthcheck. It made the whole services crash.I looked at the code, it seems without this (code removed in custom healthcheck) getting the version doesn't have any issue in staging and production.For the development side to display the version, we have to runpnpm start
which start our backend that a API route that return the version and commit for our backend.https://github.com/Seneca-CDOT/telescope/blob/master/src/backend/web/routes/health.js
Final solution
Removing the custom healthcheck in status services, and in our front-end we do the fetch directly there.
Steps to test the PR
Development
pnpm services:start
localhost:8000
Staging/Production not able to test, but technically it should work.
Checklist