feat: notify template owner on manual build failures #14262
Conversation
coderd/database/migrations/000243_notifications_template_manual_build_failure.up.sql
Outdated
Show resolved
Hide resolved
coderd/database/migrations/000243_notifications_template_manual_build_failure.up.sql
Outdated
Show resolved
Hide resolved
…manual-build-failure
| @@ -1020,7 +1020,23 @@ func (s *server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*proto. | |||
| return nil, err | |||
| } | |||
|
|
|||
| s.notifyWorkspaceBuildFailed(ctx, workspace, build) | |||
| // Only notify auto build failures | |||
There was a problem hiding this comment.
I would rather that this comment explain what classifies as an auto build (i.e. not initiated by a user)
| s.Logger.Warn(ctx, "failed to find template admins for template build failed notification", slog.Error(err)) | ||
| } else { | ||
| for _, admin := range admins { | ||
| if admin.ID != build.InitiatorID && !autoBuild { |
There was a problem hiding this comment.
| if admin.ID != build.InitiatorID && !autoBuild { | |
| // Only notify users who did not initiate the action. | |
| if admin.ID != build.InitiatorID && !autoBuild { |
| reason = string(build.Reason) | ||
|
|
||
| if _, err := s.NotificationsEnqueuer.Enqueue(ctx, workspace.OwnerID, notifications.TemplateWorkspaceAutobuildFailed, | ||
| func (s *server) notifyWorkspaceBuildFailed(ctx context.Context, workspace database.Workspace, build database.WorkspaceBuild, receiverID uuid.UUID) { |
There was a problem hiding this comment.
I don't understand why you're passing in receiverID here. Can you elaborate pls?
| func (s *server) notifyTemplateBuildFailed(ctx context.Context, workspace database.Workspace, build database.WorkspaceBuild, receiverID uuid.UUID) { | ||
| template, err := s.Database.GetTemplateByID(ctx, workspace.TemplateID) | ||
| if err != nil { | ||
| s.Logger.Warn(ctx, "failed to get template", slog.Error(err)) |
There was a problem hiding this comment.
Think about how an operator might debug with these logs; how would they know that this is related to sending notifications?
Try think of log lines as communication. You're communicating with an operator who is a) possibly not familiar with the product or less experienced, and b) trying to debug an issue, possibly under stress. Vague log lines are almost as bad as no log lines at all, sometimes worse because they can mislead or distract.
| for _, c := range tc { | ||
| t.Run(c.name, func(t *testing.T) { | ||
| // Given: a template and a workspace build | ||
| isManualBuild := c.reason == database.BuildReasonInitiator |
There was a problem hiding this comment.
Since you have this logic in multiple places, it might be worth creating a helper method.
| for _, c := range tc { | ||
| t.Run(c.name, func(t *testing.T) { | ||
| // Given: a template and a workspace build | ||
| isManualBuild := c.reason == database.BuildReasonInitiator |
There was a problem hiding this comment.
Try and group your code into logical blocks, and use whitespace for visual assistance. Think of your code as having the same visual hierarchy that you'd design in your frontends.
For example, isManualBuild is only needed lower down: group them logically.
You've got a whole lot of code below which is doing several things; add a bit of spaces and/or comments to give the reader's "eye" some space to "breathe", and group related concepts/lines together to infer relationships.
| // Then: send the template build failed notifications | ||
| var buildFailedNotifications []*testutil.Notification | ||
| for _, n := range notifEnq.Sent { | ||
| if n.TemplateID == notifications.TemplateTemplateBuildFailed { |
There was a problem hiding this comment.
What if they're not the template you're expecting?
Your tests should not only prove that something works; it should also prove that it only works in precisely the way you're expecting, and any relevant deviation should fail the test.
|
@BrunoQuaresma I will take over this PR, not sure if I create a one or modify yours, just FYI 👍 |
Sure, whatever you think is best. Just tag me on your PR so I can take a look and learn. |
|
Close in favor of #14419 |
Preview:
Related to coder/internal#16 and it depends on #14250