feat: notify template owner on manual build failures #14262
+264
−17
There are no files selected for viewing
Empty file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| INSERT INTO | ||
| notification_templates ( | ||
| id, | ||
| name, | ||
| title_template, | ||
| body_template, | ||
| "group", | ||
| actions | ||
| ) | ||
| VALUES ( | ||
| '48a9d2b9-3655-430c-a31a-2442479e7519', | ||
| 'Template Build Failure', | ||
| E'Build failed on workspace using template "{{.Labels.name}}"', | ||
| E'Hi {{.UserName}},\n\n' | ||
| 'A workspace using the template **{{.Labels.name}}** failed to build.\n\n' | ||
| '- **Version**: {{.Labels.version}}\n' | ||
| '- **Workspace**: {{.Labels.workspaceName}}\n' | ||
| '- **Transition**: {{.Labels.transition}}\n' | ||
| '{{if .Labels.initiator}}- **Initiated by**: {{.Labels.initiator}}{{end}}' | ||
| '{{if .Labels.reason}}- **Reason**: {{.Labels.reason}}{{end}}' | ||
| '\n\nYou can debug this workspace using the build logs below or contact the deployment administrator.', | ||
| 'Template Events', | ||
| '[ | ||
| { | ||
| "label": "View build", | ||
| "url": "{{ base_url }}/@{{.Labels.workspaceOwnerName}}/{{.Labels.workspaceName}}/builds/{{.Labels.buildNumber}}" | ||
| }, | ||
| { | ||
| "label": "View template", | ||
| "url": "{{ base_url }}/templates/{{.Labels.name}}" | ||
| } | ||
| ]'::jsonb | ||
| ); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ import ( | |
| "encoding/json" | ||
| "io" | ||
| "net/url" | ||
| "strconv" | ||
| "strings" | ||
| "sync" | ||
| "sync/atomic" | ||
|
|
@@ -1568,7 +1569,7 @@ func TestInsertWorkspaceResource(t *testing.T) { | |
| func TestNotifications(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| t.Run("WorkspaceDeletion", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| tests := []struct { | ||
|
|
@@ -1695,22 +1696,21 @@ func TestNotifications(t *testing.T) { | |
| } | ||
| }) | ||
|
|
||
| t.Run("WorkspaceAutoBuildFailed", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| buildReason database.BuildReason | ||
| shouldNotify bool | ||
| }{ | ||
| { | ||
| name: "InitiatedByOwner", | ||
| buildReason: database.BuildReasonInitiator, | ||
| shouldNotify: false, | ||
| }, | ||
| { | ||
| name: "InitiatedByAutoStart", | ||
| buildReason: database.BuildReasonAutostart, | ||
| shouldNotify: true, | ||
| }, | ||
|
|
@@ -1804,6 +1804,155 @@ func TestNotifications(t *testing.T) { | |
| }) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("TemplateBuildFailed", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| var ( | ||
| ctx = context.Background() | ||
| notifEnq = &testutil.FakeNotificationsEnqueuer{} | ||
| // To avoid spamming the output, ignore log errors. This test is | ||
| // designed to check a build failure, which is expected to log errors. | ||
| ignoreLogErrors = true | ||
| srv, db, ps, pd = setup(t, ignoreLogErrors, &overrides{ | ||
| notificationEnqueuer: notifEnq, | ||
| }) | ||
| // Create users with different roles to ensure that notifications are sent | ||
| // to the appropriate users | ||
| ownerA = dbgen.User(t, db, database.User{RBACRoles: []string{codersdk.RoleOwner}}) | ||
| ownerB = dbgen.User(t, db, database.User{RBACRoles: []string{codersdk.RoleOwner}}) | ||
| tplAdmin = dbgen.User(t, db, database.User{RBACRoles: []string{codersdk.RoleTemplateAdmin}}) | ||
| _ = dbgen.User(t, db, database.User{RBACRoles: []string{codersdk.RoleUserAdmin}}) | ||
| member = dbgen.User(t, db, database.User{RBACRoles: []string{codersdk.RoleMember}}) | ||
| ) | ||
|
|
||
| tc := []struct { | ||
| name string | ||
| tplOwner database.User | ||
| buildInitiator database.User | ||
| reason database.BuildReason | ||
| transition database.WorkspaceTransition | ||
| receivers []uuid.UUID | ||
| }{ | ||
| { | ||
| name: "ManualBuild", | ||
| tplOwner: ownerA, | ||
| buildInitiator: ownerB, | ||
| reason: database.BuildReasonInitiator, | ||
| transition: database.WorkspaceTransitionStart, | ||
| // Ensure that during manual builds, the initiator does not receive a | ||
| // notification. In this scenario, ownerB should not receive a | ||
| // notification. | ||
| receivers: []uuid.UUID{ownerA.ID, tplAdmin.ID}, | ||
| }, | ||
| { | ||
| name: "AutoBuild", | ||
| tplOwner: ownerA, | ||
| buildInitiator: ownerA, | ||
| reason: database.BuildReasonAutostart, | ||
| transition: database.WorkspaceTransitionStart, | ||
| // Ensure that during automated builds, all template admins and owners | ||
| // receive notifications. | ||
| receivers: []uuid.UUID{ownerA.ID, ownerB.ID, tplAdmin.ID}, | ||
| }, | ||
| } | ||
|
|
||
| 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. 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, 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. |
||
| template := dbgen.Template(t, db, database.Template{ | ||
| Name: "template", | ||
| Provisioner: database.ProvisionerTypeEcho, | ||
| OrganizationID: pd.OrganizationID, | ||
| CreatedBy: c.tplOwner.ID, | ||
| }) | ||
| template, err := db.GetTemplateByID(ctx, template.ID) | ||
| require.NoError(t, err) | ||
| version := dbgen.TemplateVersion(t, db, database.TemplateVersion{ | ||
| OrganizationID: pd.OrganizationID, | ||
| TemplateID: uuid.NullUUID{ | ||
| UUID: template.ID, | ||
| Valid: true, | ||
| }, | ||
| JobID: uuid.New(), | ||
| }) | ||
| workspaceOwner := member | ||
| workspace := dbgen.Workspace(t, db, database.Workspace{ | ||
| TemplateID: template.ID, | ||
| OwnerID: workspaceOwner.ID, | ||
| OrganizationID: pd.OrganizationID, | ||
| }) | ||
| b := database.WorkspaceBuild{ | ||
| WorkspaceID: workspace.ID, | ||
| TemplateVersionID: version.ID, | ||
| Transition: c.transition, | ||
| Reason: c.reason, | ||
| } | ||
| // Set the build initiator if the test case specifies one for manual builds. | ||
| if isManualBuild { | ||
| b.InitiatorID = c.buildInitiator.ID | ||
| } | ||
| build := dbgen.WorkspaceBuild(t, db, b) | ||
| file := dbgen.File(t, db, database.File{CreatedBy: c.buildInitiator.ID}) | ||
| job := dbgen.ProvisionerJob(t, db, ps, database.ProvisionerJob{ | ||
| FileID: file.ID, | ||
| Type: database.ProvisionerJobTypeWorkspaceBuild, | ||
| Input: must(json.Marshal(provisionerdserver.WorkspaceProvisionJob{ | ||
| WorkspaceBuildID: build.ID, | ||
| })), | ||
| OrganizationID: pd.OrganizationID, | ||
| }) | ||
| _, err = db.AcquireProvisionerJob(ctx, database.AcquireProvisionerJobParams{ | ||
| OrganizationID: pd.OrganizationID, | ||
| WorkerID: uuid.NullUUID{ | ||
| UUID: pd.ID, | ||
| Valid: true, | ||
| }, | ||
| Types: []database.ProvisionerType{database.ProvisionerTypeEcho}, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // When: the build fails | ||
| _, err = srv.FailJob(ctx, &proto.FailedJob{ | ||
| JobId: job.ID.String(), | ||
| Type: &proto.FailedJob_WorkspaceBuild_{ | ||
| WorkspaceBuild: &proto.FailedJob_WorkspaceBuild{ | ||
| State: []byte{}, | ||
| }, | ||
| }, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // 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. |
||
| buildFailedNotifications = append(buildFailedNotifications, n) | ||
| } | ||
| } | ||
| require.Len(t, buildFailedNotifications, len(c.receivers)) | ||
|
|
||
| for _, n := range buildFailedNotifications { | ||
| require.Contains(t, n.Targets, template.ID) | ||
| require.Contains(t, n.Targets, workspace.ID) | ||
| require.Contains(t, n.Targets, workspace.OrganizationID) | ||
| require.Contains(t, n.Targets, c.tplOwner.ID) | ||
|
|
||
| require.Equal(t, n.Labels["name"], template.Name) | ||
| require.Equal(t, n.Labels["version"], version.Name) | ||
| require.Equal(t, n.Labels["workspaceName"], workspace.Name) | ||
| require.Equal(t, n.Labels["transition"], string(build.Transition)) | ||
| require.Equal(t, n.Labels["reason"], string(build.Reason)) | ||
| require.Equal(t, n.Labels["workspaceOwnerName"], workspaceOwner.Username) | ||
| require.Equal(t, n.Labels["buildNumber"], strconv.FormatInt(int64(build.BuildNumber), 10)) | ||
| if isManualBuild { | ||
| require.Equal(t, n.Labels["initiator"], c.buildInitiator.Username) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| type overrides struct { | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I would rather that this comment explain what classifies as an auto build (i.e. not initiated by a user)