-
Notifications
You must be signed in to change notification settings - Fork 102
feat(backend, auth): /healthz fails if a database is not connected #3717
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for brilliant-pasca-3e80ec ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| */ | ||
| public async boot(): Promise<void> { | ||
| this.config = await this.container.use('config') | ||
| this.logger = await this.container.use('logger') |
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.
this removal seems to be resulting in the 500 errors on backend/integration/performance tests.
I had assumed the 500 errors were coming from the new healthcheck but as I traced it that didnt make much sense and reverting the healthz changes didnt fix the issue.
I guess this was probably removed in error, and the non-null assertion on the logger class property didnt help. We can simply return it.
It's a small thing, but we could probably just assign logger/config in the constructor to bypass the need for the non-null assertion. That would require changing the container singletons to not (unnecessarily?) return promises. Which should be fine. We have this pattern of making them return promises but it seems entirely unnecessary for logger, config, and most others.
export interface AppServices {
logger: Promise<Logger>
telemetry: Promise<TelemetryService>
internalRatesService: Promise<RatesService>
knex: Promise<Knex>
axios: Promise<AxiosInstance>
config: Promise<IAppConfig>
httpTokenService: Promise<HttpTokenService>
... etcThen that makes it looks like maybe we can even just move the entire boot function to the constructor, since it no longer needs to be async. That would also change some timing though (currently we do constructor, then other stuff, then boot) but I dont think that should matter. Guess it's a little can of worms and we should at least just return the line here.
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.
Yeah for now I'll just reintroduce the deleted line.
🚀 Performance Test ResultsTest Configuration:
Test Metrics:
📜 Logs |
Changes proposed in this pull request
/healthzendpoint, both thebackendandauthservices will try to reach both its postgres database and its redis instance and responding appropriately based on the result.Context
Closes RAF-1172.
Checklist
fixes #numberuser-docslabel (if necessary)