-
Notifications
You must be signed in to change notification settings - Fork 281
fix(breachAlerts): update cron job to autoscaling service #6287
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
Remove the fixed message processing count and batch pulls. Do not accept new messages after termination signals Refactor job with DI for easier unit testing
The imports and mocks in tests seem to have masked missing coverage, and moving to DI pattern revealed gaps once they were no longer imported
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| import { logger } from "@sentry/utils"; | ||
| import { logger } from "@sentry/core"; |
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 was deprecated, recommended to use @sentry/core
| export const redisClient = () => { | ||
| if (process.env.REDIS_URL?.includes("redis.mock")) { | ||
| singleton = createRedisMockInstance(); | ||
| singleton = new MockRedis(); |
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 was a one-liner in utils-mock file, which I moved here for clarity and to eliminate needing another test for that file
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| import { createRedisInstance } from "./util"; |
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.
added due to coverage complaint (probably from removing this import in the test files?)
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| import { isUsingMockHIBPEndpoint, isUsingMockONEREPEndpoint } from "./mock"; |
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.
added due to coverage complaint (no changes to the files)
|
|
||
| import { parseMarkup } from "./parseMarkup"; | ||
|
|
||
| describe("parseMarkup", () => { |
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.
added due to coverage complaint (no changes to method)
|
|
||
| export function isUsingMockHIBPEndpoint() { | ||
| return process.env.HIBP_KANON_API_ROOT?.includes("api/mock") as boolean; | ||
| return !!process.env.HIBP_KANON_API_ROOT?.includes("api/mock"); |
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 was returning undefined if HIBP_KANON_API_ROOT was unset; !! is idiomatic boolean conversion
References:
Jira:
Required for: https://mozilla-hub.atlassian.net/browse/SVCSE-2984
Description
Updates the breach alerts email so that it can be compatible with autoscaling up and down based on queue size rather than run as a cron job. This required major pattern changes to how the job was invoked.
I refactored the job so that the pubsub subscription handling logic was separated from the business logic/processing for each message. I also updated the job to use DI pattern for easier testing. Much of the core message processing logic was lifted, with some streamlining around db checks. Since this was such a big overhaul I included some low-hanging fruit (removing premium upsell [5108] and removing the nimbus integration [5081]).
All the methods are unit-tested except for the main
jobentrypoint, which I had originally tried to cover with an integration test but couldn't run injsdomenvironment because pubsub client requiressetImmediate, etc. I tested this locally successfully and will do a dev deployment.Other Changes