- Notifications
You must be signed in to change notification settings - Fork1.1k
feat: notifications: report failed workspace builds#14571
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Alright, I addressed most of the feedback. Important changes:
|
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.
LGTM
Approving to unblock, but I really feel we should try add some testing aroundNewReportGenerator. I'm happy to land this first and have a follow-up PR which just focuses on that, since it could get complicated.
Excited to see this in action!
Uh oh!
There was an error while loading.Please reload this page.
| delay = 15 * time.Minute | ||
| ) | ||
| func NewReportGenerator(ctx context.Context, logger slog.Logger, db database.Store, enqueuer notifications.Enqueuer, clk quartz.Clock) io.Closer { |
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.
What is not tested using the current approach is exclusive locking, and this will be hard.
What makes this particularly hard (asking for real, not rhetorically)? To me it seems like we'd need to invokeNewReportGenerator in two separate goroutines and just ensure that we only get what we're expecting, which you've implemented in other tests.
I think it's quite risky to have functionality which should be guarding against pathological behaviour which is not covered by a test.
Uh oh!
There was an error while loading.Please reload this page.
Fixes:coder/internal#40
This PR introduces support for email reports, including a report with failed workspace builds. A report generator runs periodically to see if users expect a new report. Once the report is successfully generated and spread, a record (or records) are logged in the database table.
In the multi-instance setup, report generators use an exclusive locking mechanism to ensure only one instance is running.