Conversation
ajhollid
requested review from
Br0wnHammer,
Owaiseimdad and
karenvicent
as code owners
|
Yesss!!!!!!! |
Br0wnHammer
left a comment
Br0wnHammer
left a comment
There was a problem hiding this comment.
Solid work 🚀. Please see my comments.
| onLocationChange(newValue); | ||
| }; | ||
|
|
||
| console.log(locations); |
| pauseJob = async (monitor: any) => { | ||
| const result = await this.scheduler.pauseJob(monitor.id); | ||
| if (result === false) { | ||
| throw new Error("Failed to resume monitor"); |
There was a problem hiding this comment.
Nit: Error message is wrong.
| if (result === false) { | ||
| throw new Error("Failed to resume monitor"); | ||
| } | ||
| const geoResult = await this.scheduler.resumeJob(`${monitor.id}-geo`); |
There was a problem hiding this comment.
resumeJob always attempts to resume ${monitor.id}-geo and throws if it returns false. This will break all monitors without geoCheckEnabled, since no geo job exists for them.
|
|
||
| const SERVICE_NAME = "GeoChecksRepository"; | ||
|
|
||
| const dateRangeLookup: Record<string, Date> = { |
There was a problem hiding this comment.
These Date values are computed at import time and never refreshed. After the service has been running for a while:
recent is no longer “last hour”
it’s “last hour relative to server startup”
So after a few hours/days, this becomes silently wrong.
There was a problem hiding this comment.
Good call, this has gone unnoticed for some time
|
|
||
| const SERVICE_NAME = "GeoChecksRepository"; | ||
|
|
||
| const dateRangeLookup: Record<string, Date> = { |
There was a problem hiding this comment.
The Joi validation allows dateRange: "hour", but the repository dateRangeLookup only has recent, day, week, month. Passing "hour" will result in no date filter being applied (it'll return all data).
There was a problem hiding this comment.
Joi validation is due for replacement with zod, we can remove this at the same time
3 participants
This feature PR integrates the GlobalPing API into Checkmate