From 450db4d4eeb4ab9b4ce43a1889dd81c90de5c12c Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 15 Jul 2024 13:13:44 +0200 Subject: [PATCH 01/13] feat: notify owner about failed autobuild --- ...26_notifications_autobuild_failed.down.sql | 1 + ...0226_notifications_autobuild_failed.up.sql | 9 +++++++ coderd/notifications/events.go | 5 +++- .../provisionerdserver/provisionerdserver.go | 27 +++++++++++++++++++ 4 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 coderd/database/migrations/000226_notifications_autobuild_failed.down.sql create mode 100644 coderd/database/migrations/000226_notifications_autobuild_failed.up.sql diff --git a/coderd/database/migrations/000226_notifications_autobuild_failed.down.sql b/coderd/database/migrations/000226_notifications_autobuild_failed.down.sql new file mode 100644 index 0000000000000..a83dd2d0089ee --- /dev/null +++ b/coderd/database/migrations/000226_notifications_autobuild_failed.down.sql @@ -0,0 +1 @@ +DELETE FROM notification_templates WHERE id = '381df2a9-c0c0-4749-420f-80a9280c66f9'); diff --git a/coderd/database/migrations/000226_notifications_autobuild_failed.up.sql b/coderd/database/migrations/000226_notifications_autobuild_failed.up.sql new file mode 100644 index 0000000000000..8f869c4e511ea --- /dev/null +++ b/coderd/database/migrations/000226_notifications_autobuild_failed.up.sql @@ -0,0 +1,9 @@ +INSERT INTO notification_templates (id, name, title_template, body_template, "group", actions) +VALUES ('381df2a9-c0c0-4749-420f-80a9280c66f9', 'Workspace Autobuild Failed', E'Workspace "{{.Labels.name}}" deleted', + E'Hi {{.UserName}}\n\Automatic build of your workspace **{{.Labels.name}}** failed.\nThe specified reason was "**{{.Labels.reason}}**".', + 'Workspace Events', '[ + { + "label": "View workspaces", + "url": "{{ base_url }}/workspaces" + } + ]'::jsonb); diff --git a/coderd/notifications/events.go b/coderd/notifications/events.go index 6cb2870748b61..59ff87f67eef9 100644 --- a/coderd/notifications/events.go +++ b/coderd/notifications/events.go @@ -6,4 +6,7 @@ import "github.com/google/uuid" // TODO: autogenerate these. // Workspace-related events. -var TemplateWorkspaceDeleted = uuid.MustParse("f517da0b-cdc9-410f-ab89-a86107c420ed") +var ( + TemplateWorkspaceDeleted = uuid.MustParse("f517da0b-cdc9-410f-ab89-a86107c420ed") + WorkspaceAutobuildFailed = uuid.MustParse("381df2a9-c0c0-4749-420f-80a9280c66f9") +) diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 79185862daa2e..fa0fde959c93e 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -982,12 +982,18 @@ func (s *server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*proto. } var build database.WorkspaceBuild + var workspace database.Workspace err = s.Database.InTx(func(db database.Store) error { build, err = db.GetWorkspaceBuildByID(ctx, input.WorkspaceBuildID) if err != nil { return xerrors.Errorf("get workspace build: %w", err) } + workspace, err = db.GetWorkspaceByID(ctx, build.WorkspaceID) + if err != nil { + return xerrors.Errorf("get workspace: %w", err) + } + if jobType.WorkspaceBuild.State != nil { err = db.UpdateWorkspaceBuildProvisionerStateByID(ctx, database.UpdateWorkspaceBuildProvisionerStateByIDParams{ ID: input.WorkspaceBuildID, @@ -1014,6 +1020,8 @@ func (s *server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*proto. return nil, err } + s.notifyWorkspaceBuildFailed(ctx, workspace, build) + err = s.Pubsub.Publish(codersdk.WorkspaceNotifyChannel(build.WorkspaceID), []byte{}) if err != nil { return nil, xerrors.Errorf("update workspace: %w", err) @@ -1087,6 +1095,25 @@ func (s *server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*proto. return &proto.Empty{}, nil } +func (s *server) notifyWorkspaceBuildFailed(ctx context.Context, workspace database.Workspace, build database.WorkspaceBuild) { + var reason string + if build.Reason.Valid() && build.Reason == database.BuildReasonInitiator { + return // failed workspace builds initiated by a user should not notify + } + + if _, err := s.NotificationEnqueuer.Enqueue(ctx, workspace.OwnerID, notifications.WorkspaceAutobuildFailed, + map[string]string{ + "name": workspace.Name, + "initiator": build.InitiatorByUsername, + "reason": reason, + }, "provisionerdserver", + // Associate this notification with all the related entities. + workspace.ID, workspace.OwnerID, workspace.TemplateID, workspace.OrganizationID, + ); err != nil { + s.Logger.Warn(ctx, "failed to notify of failed workspace autobuild", slog.Error(err)) + } +} + // CompleteJob is triggered by a provision daemon to mark a provisioner job as completed. func (s *server) CompleteJob(ctx context.Context, completed *proto.CompletedJob) (*proto.Empty, error) { ctx, span := s.startTrace(ctx, tracing.FuncName()) From 422fe8b04a53d4ac3c3a679afc8d397a5444eeee Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 15 Jul 2024 13:27:12 +0200 Subject: [PATCH 02/13] fix: migrate down --- .../migrations/000226_notifications_autobuild_failed.down.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/database/migrations/000226_notifications_autobuild_failed.down.sql b/coderd/database/migrations/000226_notifications_autobuild_failed.down.sql index a83dd2d0089ee..6695445a90238 100644 --- a/coderd/database/migrations/000226_notifications_autobuild_failed.down.sql +++ b/coderd/database/migrations/000226_notifications_autobuild_failed.down.sql @@ -1 +1 @@ -DELETE FROM notification_templates WHERE id = '381df2a9-c0c0-4749-420f-80a9280c66f9'); +DELETE FROM notification_templates WHERE id = '381df2a9-c0c0-4749-420f-80a9280c66f9'; From 8970be7d36fbc0cbc3188586fef722191213823f Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 15 Jul 2024 14:00:50 +0200 Subject: [PATCH 03/13] unit test --- .../provisionerdserver/provisionerdserver.go | 6 +- .../provisionerdserver_test.go | 90 ++++++++++++------- 2 files changed, 61 insertions(+), 35 deletions(-) diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index fa0fde959c93e..caffc0d0b8eba 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -1572,9 +1572,9 @@ func (s *server) notifyWorkspaceDeleted(ctx context.Context, workspace database. if _, err := s.NotificationEnqueuer.Enqueue(ctx, workspace.OwnerID, notifications.TemplateWorkspaceDeleted, map[string]string{ - "name": workspace.Name, - "initiatedBy": build.InitiatorByUsername, - "reason": reason, + "name": workspace.Name, + "initiator": build.InitiatorByUsername, + "reason": reason, }, "provisionerdserver", // Associate this notification with all the related entities. workspace.ID, workspace.OwnerID, workspace.TemplateID, workspace.OrganizationID, diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index 7049359be98a7..e0165879a6dc6 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -1569,31 +1569,42 @@ func TestInsertWorkspaceResource(t *testing.T) { func TestNotifications(t *testing.T) { t.Parallel() - t.Run("Workspace deletion", func(t *testing.T) { + t.Run("Workspace Events", func(t *testing.T) { t.Parallel() tests := []struct { - name string - deletionReason database.BuildReason - shouldNotify bool - shouldSelfInitiate bool + name string + + buildReason database.BuildReason + buildFailed bool + shouldNotify bool + shouldSelfInitiate bool + shouldDeleteWorkspace bool }{ { - name: "initiated by autodelete", - deletionReason: database.BuildReasonAutodelete, - shouldNotify: true, + name: "initiated by autodelete", + buildReason: database.BuildReasonAutodelete, + shouldNotify: true, + shouldDeleteWorkspace: true, + }, + { + name: "initiated by self", + buildReason: database.BuildReasonInitiator, + shouldNotify: false, + shouldSelfInitiate: true, + shouldDeleteWorkspace: true, }, { - name: "initiated by self", - deletionReason: database.BuildReasonInitiator, - shouldNotify: false, - shouldSelfInitiate: true, + name: "initiated by someone else", + buildReason: database.BuildReasonInitiator, + shouldNotify: true, + shouldDeleteWorkspace: true, }, { - name: "initiated by someone else", - deletionReason: database.BuildReasonInitiator, - shouldNotify: true, - shouldSelfInitiate: false, + name: "initiated by autostart but failed", + buildReason: database.BuildReasonAutostart, + buildFailed: true, + shouldNotify: true, }, } @@ -1604,7 +1615,11 @@ func TestNotifications(t *testing.T) { ctx := context.Background() notifEnq := &fakeNotificationEnqueuer{} - srv, db, ps, pd := setup(t, false, &overrides{ + // Otherwise `(*Server).FailJob` fails with: + // audit log - get build {"error": "sql: no rows in result set"} + ignoreLogErrors := tc.buildFailed + + srv, db, ps, pd := setup(t, ignoreLogErrors, &overrides{ notificationEnqueuer: notifEnq, }) @@ -1640,7 +1655,7 @@ func TestNotifications(t *testing.T) { TemplateVersionID: version.ID, InitiatorID: initiator.ID, Transition: database.WorkspaceTransitionDelete, - Reason: tc.deletionReason, + Reason: tc.buildReason, }) job := dbgen.ProvisionerJob(t, db, ps, database.ProvisionerJob{ FileID: file.ID, @@ -1660,23 +1675,34 @@ func TestNotifications(t *testing.T) { }) require.NoError(t, err) - _, err = srv.CompleteJob(ctx, &proto.CompletedJob{ - JobId: job.ID.String(), - Type: &proto.CompletedJob_WorkspaceBuild_{ - WorkspaceBuild: &proto.CompletedJob_WorkspaceBuild{ - State: []byte{}, - Resources: []*sdkproto.Resource{{ - Name: "example", - Type: "aws_instance", - }}, + if tc.buildFailed { + _, err = srv.FailJob(ctx, &proto.FailedJob{ + JobId: job.ID.String(), + Type: &proto.FailedJob_WorkspaceBuild_{ + WorkspaceBuild: &proto.FailedJob_WorkspaceBuild{ + State: []byte{}, + }, }, - }, - }) + }) + } else { + _, err = srv.CompleteJob(ctx, &proto.CompletedJob{ + JobId: job.ID.String(), + Type: &proto.CompletedJob_WorkspaceBuild_{ + WorkspaceBuild: &proto.CompletedJob_WorkspaceBuild{ + State: []byte{}, + Resources: []*sdkproto.Resource{{ + Name: "example", + Type: "aws_instance", + }}, + }, + }, + }) + } require.NoError(t, err) workspace, err = db.GetWorkspaceByID(ctx, workspace.ID) require.NoError(t, err) - require.True(t, workspace.Deleted) + require.Equal(t, tc.shouldDeleteWorkspace, workspace.Deleted) if tc.shouldNotify { // Validate that the notification was sent and contained the expected values. @@ -1686,8 +1712,8 @@ func TestNotifications(t *testing.T) { require.Contains(t, notifEnq.sent[0].targets, workspace.ID) require.Contains(t, notifEnq.sent[0].targets, workspace.OrganizationID) require.Contains(t, notifEnq.sent[0].targets, user.ID) - if tc.deletionReason == database.BuildReasonInitiator { - require.Equal(t, notifEnq.sent[0].labels["initiatedBy"], initiator.Username) + if tc.buildReason == database.BuildReasonInitiator { + require.Equal(t, notifEnq.sent[0].labels["initiator"], initiator.Username) } } else { require.Len(t, notifEnq.sent, 0) From 3ace8c679b82df058147b1991e93b6b35bdfc466 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 15 Jul 2024 14:12:52 +0200 Subject: [PATCH 04/13] fix --- coderd/provisionerdserver/provisionerdserver.go | 5 ++--- coderd/provisionerdserver/provisionerdserver_test.go | 3 +++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index caffc0d0b8eba..9cfc852279400 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -1103,9 +1103,8 @@ func (s *server) notifyWorkspaceBuildFailed(ctx context.Context, workspace datab if _, err := s.NotificationEnqueuer.Enqueue(ctx, workspace.OwnerID, notifications.WorkspaceAutobuildFailed, map[string]string{ - "name": workspace.Name, - "initiator": build.InitiatorByUsername, - "reason": reason, + "name": workspace.Name, + "reason": reason, }, "provisionerdserver", // Associate this notification with all the related entities. workspace.ID, workspace.OwnerID, workspace.TemplateID, workspace.OrganizationID, diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index e0165879a6dc6..001e44e0b6ee7 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -1714,6 +1714,9 @@ func TestNotifications(t *testing.T) { require.Contains(t, notifEnq.sent[0].targets, user.ID) if tc.buildReason == database.BuildReasonInitiator { require.Equal(t, notifEnq.sent[0].labels["initiator"], initiator.Username) + } else { + _, ok := notifEnq.sent[0].labels["initiator"] + require.False(t, ok, "initiator label not expected") } } else { require.Len(t, notifEnq.sent, 0) From 1001bc14a816ca53da6250f2560ae7178d27fdbf Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 15 Jul 2024 14:14:18 +0200 Subject: [PATCH 05/13] fix template title --- .../migrations/000226_notifications_autobuild_failed.up.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/database/migrations/000226_notifications_autobuild_failed.up.sql b/coderd/database/migrations/000226_notifications_autobuild_failed.up.sql index 8f869c4e511ea..2e0694fdab8fa 100644 --- a/coderd/database/migrations/000226_notifications_autobuild_failed.up.sql +++ b/coderd/database/migrations/000226_notifications_autobuild_failed.up.sql @@ -1,5 +1,5 @@ INSERT INTO notification_templates (id, name, title_template, body_template, "group", actions) -VALUES ('381df2a9-c0c0-4749-420f-80a9280c66f9', 'Workspace Autobuild Failed', E'Workspace "{{.Labels.name}}" deleted', +VALUES ('381df2a9-c0c0-4749-420f-80a9280c66f9', 'Workspace Autobuild Failed', E'Workspace "{{.Labels.name}}" autobuild failed', E'Hi {{.UserName}}\n\Automatic build of your workspace **{{.Labels.name}}** failed.\nThe specified reason was "**{{.Labels.reason}}**".', 'Workspace Events', '[ { From 881dc5e7ff8c6b72003fd91fc9aeb1ab153f29b4 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 15 Jul 2024 14:34:12 +0200 Subject: [PATCH 06/13] fix: initiator --- coderd/provisionerdserver/provisionerdserver.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 9cfc852279400..579e3c95ba5b2 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -1549,6 +1549,7 @@ func (s *server) CompleteJob(ctx context.Context, completed *proto.CompletedJob) func (s *server) notifyWorkspaceDeleted(ctx context.Context, workspace database.Workspace, build database.WorkspaceBuild) { var reason string + initiator := build.InitiatorByUsername if build.Reason.Valid() { switch build.Reason { case database.BuildReasonInitiator: @@ -1560,6 +1561,7 @@ func (s *server) notifyWorkspaceDeleted(ctx context.Context, workspace database. reason = "initiated by user" case database.BuildReasonAutodelete: reason = "autodeleted due to dormancy" + initiator = "" default: reason = string(build.Reason) } @@ -1569,12 +1571,17 @@ func (s *server) notifyWorkspaceDeleted(ctx context.Context, workspace database. slog.F("reason", reason), slog.F("workspace_id", workspace.ID), slog.F("build_id", build.ID)) } + labels := map[string]string{ + "name": workspace.Name, + "reason": reason, + } + + if initiator != "" { + labels["initiator"] = initiator + } + if _, err := s.NotificationEnqueuer.Enqueue(ctx, workspace.OwnerID, notifications.TemplateWorkspaceDeleted, - map[string]string{ - "name": workspace.Name, - "initiator": build.InitiatorByUsername, - "reason": reason, - }, "provisionerdserver", + labels, "provisionerdserver", // Associate this notification with all the related entities. workspace.ID, workspace.OwnerID, workspace.TemplateID, workspace.OrganizationID, ); err != nil { From 18529536f989914bd23856cdeedb82e05710b968 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 15 Jul 2024 15:36:48 +0200 Subject: [PATCH 07/13] WIP --- .../provisionerdserver/provisionerdserver.go | 26 ++- .../provisionerdserver_test.go | 191 +++++++++++++----- 2 files changed, 148 insertions(+), 69 deletions(-) diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 579e3c95ba5b2..2c7b311d1ee84 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -1098,13 +1098,16 @@ func (s *server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*proto. func (s *server) notifyWorkspaceBuildFailed(ctx context.Context, workspace database.Workspace, build database.WorkspaceBuild) { var reason string if build.Reason.Valid() && build.Reason == database.BuildReasonInitiator { - return // failed workspace builds initiated by a user should not notify + return // failed workspace build initiated by a user should not notify } + reason = "initiated by autobuild" + initiator := "autobuild" if _, err := s.NotificationEnqueuer.Enqueue(ctx, workspace.OwnerID, notifications.WorkspaceAutobuildFailed, map[string]string{ - "name": workspace.Name, - "reason": reason, + "name": workspace.Name, + "initiator": initiator, + "reason": reason, }, "provisionerdserver", // Associate this notification with all the related entities. workspace.ID, workspace.OwnerID, workspace.TemplateID, workspace.OrganizationID, @@ -1561,7 +1564,7 @@ func (s *server) notifyWorkspaceDeleted(ctx context.Context, workspace database. reason = "initiated by user" case database.BuildReasonAutodelete: reason = "autodeleted due to dormancy" - initiator = "" + initiator = "autobuild" default: reason = string(build.Reason) } @@ -1571,17 +1574,12 @@ func (s *server) notifyWorkspaceDeleted(ctx context.Context, workspace database. slog.F("reason", reason), slog.F("workspace_id", workspace.ID), slog.F("build_id", build.ID)) } - labels := map[string]string{ - "name": workspace.Name, - "reason": reason, - } - - if initiator != "" { - labels["initiator"] = initiator - } - if _, err := s.NotificationEnqueuer.Enqueue(ctx, workspace.OwnerID, notifications.TemplateWorkspaceDeleted, - labels, "provisionerdserver", + map[string]string{ + "name": workspace.Name, + "reason": reason, + "initiator": initiator, + }, "provisionerdserver", // Associate this notification with all the related entities. workspace.ID, workspace.OwnerID, workspace.TemplateID, workspace.OrganizationID, ); err != nil { diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index 001e44e0b6ee7..ba50603d46094 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -1569,41 +1569,146 @@ func TestInsertWorkspaceResource(t *testing.T) { func TestNotifications(t *testing.T) { t.Parallel() - t.Run("Workspace Events", func(t *testing.T) { + t.Run("Workspace deletion", func(t *testing.T) { t.Parallel() tests := []struct { - name string - - buildReason database.BuildReason - buildFailed bool - shouldNotify bool - shouldSelfInitiate bool - shouldDeleteWorkspace bool + name string + deletionReason database.BuildReason + shouldNotify bool + shouldSelfInitiate bool }{ { - name: "initiated by autodelete", - buildReason: database.BuildReasonAutodelete, - shouldNotify: true, - shouldDeleteWorkspace: true, + name: "initiated by autodelete", + deletionReason: database.BuildReasonAutodelete, + shouldNotify: true, }, { - name: "initiated by self", - buildReason: database.BuildReasonInitiator, - shouldNotify: false, - shouldSelfInitiate: true, - shouldDeleteWorkspace: true, + name: "initiated by self", + deletionReason: database.BuildReasonInitiator, + shouldNotify: false, + shouldSelfInitiate: true, }, { - name: "initiated by someone else", - buildReason: database.BuildReasonInitiator, - shouldNotify: true, - shouldDeleteWorkspace: true, + name: "initiated by someone else", + deletionReason: database.BuildReasonInitiator, + shouldNotify: true, + shouldSelfInitiate: false, }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + notifEnq := &fakeNotificationEnqueuer{} + + srv, db, ps, pd := setup(t, false, &overrides{ + notificationEnqueuer: notifEnq, + }) + + user := dbgen.User(t, db, database.User{}) + initiator := user + if !tc.shouldSelfInitiate { + initiator = dbgen.User(t, db, database.User{}) + } + + template := dbgen.Template(t, db, database.Template{ + Name: "template", + Provisioner: database.ProvisionerTypeEcho, + OrganizationID: pd.OrganizationID, + }) + template, err := db.GetTemplateByID(ctx, template.ID) + require.NoError(t, err) + file := dbgen.File(t, db, database.File{CreatedBy: user.ID}) + workspace := dbgen.Workspace(t, db, database.Workspace{ + TemplateID: template.ID, + OwnerID: user.ID, + OrganizationID: pd.OrganizationID, + }) + version := dbgen.TemplateVersion(t, db, database.TemplateVersion{ + OrganizationID: pd.OrganizationID, + TemplateID: uuid.NullUUID{ + UUID: template.ID, + Valid: true, + }, + JobID: uuid.New(), + }) + build := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ + WorkspaceID: workspace.ID, + TemplateVersionID: version.ID, + InitiatorID: initiator.ID, + Transition: database.WorkspaceTransitionDelete, + Reason: tc.deletionReason, + }) + 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) + + _, err = srv.CompleteJob(ctx, &proto.CompletedJob{ + JobId: job.ID.String(), + Type: &proto.CompletedJob_WorkspaceBuild_{ + WorkspaceBuild: &proto.CompletedJob_WorkspaceBuild{ + State: []byte{}, + Resources: []*sdkproto.Resource{{ + Name: "example", + Type: "aws_instance", + }}, + }, + }, + }) + require.NoError(t, err) + + workspace, err = db.GetWorkspaceByID(ctx, workspace.ID) + require.NoError(t, err) + require.True(t, workspace.Deleted) + + if tc.shouldNotify { + // Validate that the notification was sent and contained the expected values. + require.Len(t, notifEnq.sent, 1) + require.Equal(t, notifEnq.sent[0].userID, user.ID) + require.Contains(t, notifEnq.sent[0].targets, template.ID) + require.Contains(t, notifEnq.sent[0].targets, workspace.ID) + require.Contains(t, notifEnq.sent[0].targets, workspace.OrganizationID) + require.Contains(t, notifEnq.sent[0].targets, user.ID) + if tc.deletionReason == database.BuildReasonInitiator { + require.Equal(t, notifEnq.sent[0].labels["initiator"], initiator.Username) + } + } else { + require.Len(t, notifEnq.sent, 0) + } + }) + } + }) + + t.Run("Workspace autobuild failed", func(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + + buildReason database.BuildReason + shouldNotify bool + shouldDeleteWorkspace bool + }{ { name: "initiated by autostart but failed", buildReason: database.BuildReasonAutostart, - buildFailed: true, shouldNotify: true, }, } @@ -1617,17 +1722,13 @@ func TestNotifications(t *testing.T) { // Otherwise `(*Server).FailJob` fails with: // audit log - get build {"error": "sql: no rows in result set"} - ignoreLogErrors := tc.buildFailed - + ignoreLogErrors := true srv, db, ps, pd := setup(t, ignoreLogErrors, &overrides{ notificationEnqueuer: notifEnq, }) user := dbgen.User(t, db, database.User{}) initiator := user - if !tc.shouldSelfInitiate { - initiator = dbgen.User(t, db, database.User{}) - } template := dbgen.Template(t, db, database.Template{ Name: "template", @@ -1675,29 +1776,14 @@ func TestNotifications(t *testing.T) { }) require.NoError(t, err) - if tc.buildFailed { - _, err = srv.FailJob(ctx, &proto.FailedJob{ - JobId: job.ID.String(), - Type: &proto.FailedJob_WorkspaceBuild_{ - WorkspaceBuild: &proto.FailedJob_WorkspaceBuild{ - State: []byte{}, - }, - }, - }) - } else { - _, err = srv.CompleteJob(ctx, &proto.CompletedJob{ - JobId: job.ID.String(), - Type: &proto.CompletedJob_WorkspaceBuild_{ - WorkspaceBuild: &proto.CompletedJob_WorkspaceBuild{ - State: []byte{}, - Resources: []*sdkproto.Resource{{ - Name: "example", - Type: "aws_instance", - }}, - }, + _, err = srv.FailJob(ctx, &proto.FailedJob{ + JobId: job.ID.String(), + Type: &proto.FailedJob_WorkspaceBuild_{ + WorkspaceBuild: &proto.FailedJob_WorkspaceBuild{ + State: []byte{}, }, - }) - } + }, + }) require.NoError(t, err) workspace, err = db.GetWorkspaceByID(ctx, workspace.ID) @@ -1712,12 +1798,7 @@ func TestNotifications(t *testing.T) { require.Contains(t, notifEnq.sent[0].targets, workspace.ID) require.Contains(t, notifEnq.sent[0].targets, workspace.OrganizationID) require.Contains(t, notifEnq.sent[0].targets, user.ID) - if tc.buildReason == database.BuildReasonInitiator { - require.Equal(t, notifEnq.sent[0].labels["initiator"], initiator.Username) - } else { - _, ok := notifEnq.sent[0].labels["initiator"] - require.False(t, ok, "initiator label not expected") - } + require.Equal(t, notifEnq.sent[0].labels["initiator"], "autobuild") } else { require.Len(t, notifEnq.sent, 0) } From 42a1c695a9a23c73817fbd604534f27db14b27f7 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 15 Jul 2024 15:46:11 +0200 Subject: [PATCH 08/13] fix: test self-initiated --- coderd/provisionerdserver/provisionerdserver_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index ba50603d46094..bc6634deacf90 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -1696,7 +1696,7 @@ func TestNotifications(t *testing.T) { } }) - t.Run("Workspace autobuild failed", func(t *testing.T) { + t.Run("Workspace build failed", func(t *testing.T) { t.Parallel() tests := []struct { @@ -1707,7 +1707,12 @@ func TestNotifications(t *testing.T) { shouldDeleteWorkspace bool }{ { - name: "initiated by autostart but failed", + name: "initiated by owner", + buildReason: database.BuildReasonInitiator, + shouldNotify: false, + }, + { + name: "initiated by autostart", buildReason: database.BuildReasonAutostart, shouldNotify: true, }, From e6a5aef55cbd2a85d32b6593e5752f9984cb3e7c Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 15 Jul 2024 16:05:51 +0200 Subject: [PATCH 09/13] fix: build reason --- coderd/provisionerdserver/provisionerdserver.go | 2 +- coderd/provisionerdserver/provisionerdserver_test.go | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 2c7b311d1ee84..e8ec371b1c354 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -1100,7 +1100,7 @@ func (s *server) notifyWorkspaceBuildFailed(ctx context.Context, workspace datab if build.Reason.Valid() && build.Reason == database.BuildReasonInitiator { return // failed workspace build initiated by a user should not notify } - reason = "initiated by autobuild" + reason = string(build.Reason) initiator := "autobuild" if _, err := s.NotificationEnqueuer.Enqueue(ctx, workspace.OwnerID, notifications.WorkspaceAutobuildFailed, diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index bc6634deacf90..ffdcf83902865 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -1702,9 +1702,8 @@ func TestNotifications(t *testing.T) { tests := []struct { name string - buildReason database.BuildReason - shouldNotify bool - shouldDeleteWorkspace bool + buildReason database.BuildReason + shouldNotify bool }{ { name: "initiated by owner", @@ -1793,7 +1792,6 @@ func TestNotifications(t *testing.T) { workspace, err = db.GetWorkspaceByID(ctx, workspace.ID) require.NoError(t, err) - require.Equal(t, tc.shouldDeleteWorkspace, workspace.Deleted) if tc.shouldNotify { // Validate that the notification was sent and contained the expected values. From a4a5511f9c97d1ae2f5b301ef2e1a13907c954ed Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 15 Jul 2024 16:42:24 +0200 Subject: [PATCH 10/13] fix: copy-paste --- coderd/provisionerdserver/provisionerdserver_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index ffdcf83902865..a04d08d03c7a5 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -1790,9 +1790,6 @@ func TestNotifications(t *testing.T) { }) require.NoError(t, err) - workspace, err = db.GetWorkspaceByID(ctx, workspace.ID) - require.NoError(t, err) - if tc.shouldNotify { // Validate that the notification was sent and contained the expected values. require.Len(t, notifEnq.sent, 1) From 660c2d6915bda530bfa4a41b7b8e8469951eeb23 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Tue, 16 Jul 2024 09:44:25 +0200 Subject: [PATCH 11/13] fix: test build reason --- coderd/provisionerdserver/provisionerdserver_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index a04d08d03c7a5..915a3f2042d55 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -1799,6 +1799,7 @@ func TestNotifications(t *testing.T) { require.Contains(t, notifEnq.sent[0].targets, workspace.OrganizationID) require.Contains(t, notifEnq.sent[0].targets, user.ID) require.Equal(t, notifEnq.sent[0].labels["initiator"], "autobuild") + require.Equal(t, notifEnq.sent[0].labels["reason"], string(tc.buildReason)) } else { require.Len(t, notifEnq.sent, 0) } From 68ae258ade868b9d813a2233b0657edd942787a4 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Tue, 16 Jul 2024 10:25:12 +0200 Subject: [PATCH 12/13] render template --- ...0226_notifications_autobuild_failed.up.sql | 4 +-- coderd/notifications/enqueuer.go | 32 ++++++++++--------- coderd/notifications/render/gotmpl_test.go | 21 +++++++++++- .../provisionerdserver_test.go | 6 ++-- 4 files changed, 42 insertions(+), 21 deletions(-) diff --git a/coderd/database/migrations/000226_notifications_autobuild_failed.up.sql b/coderd/database/migrations/000226_notifications_autobuild_failed.up.sql index 2e0694fdab8fa..d5c2f3f4824fb 100644 --- a/coderd/database/migrations/000226_notifications_autobuild_failed.up.sql +++ b/coderd/database/migrations/000226_notifications_autobuild_failed.up.sql @@ -3,7 +3,7 @@ VALUES ('381df2a9-c0c0-4749-420f-80a9280c66f9', 'Workspace Autobuild Failed', E' E'Hi {{.UserName}}\n\Automatic build of your workspace **{{.Labels.name}}** failed.\nThe specified reason was "**{{.Labels.reason}}**".', 'Workspace Events', '[ { - "label": "View workspaces", - "url": "{{ base_url }}/workspaces" + "label": "View workspace", + "url": "{{ base_url }}/@{{.UserName}}/{{.Labels.name}}" } ]'::jsonb); diff --git a/coderd/notifications/enqueuer.go b/coderd/notifications/enqueuer.go index 8838ba9be1949..d73826142f7ca 100644 --- a/coderd/notifications/enqueuer.go +++ b/coderd/notifications/enqueuer.go @@ -80,7 +80,7 @@ func (s *StoreEnqueuer) Enqueue(ctx context.Context, userID, templateID uuid.UUI // buildPayload creates the payload that the notification will for variable substitution and/or routing. // The payload contains information about the recipient, the event that triggered the notification, and any subsequent // actions which can be taken by the recipient. -func (s *StoreEnqueuer) buildPayload(ctx context.Context, userID uuid.UUID, templateID uuid.UUID, labels map[string]string) (*types.MessagePayload, error) { +func (s *StoreEnqueuer) buildPayload(ctx context.Context, userID, templateID uuid.UUID, labels map[string]string) (*types.MessagePayload, error) { metadata, err := s.store.FetchNewMessageMetadata(ctx, database.FetchNewMessageMetadataParams{ UserID: userID, NotificationTemplateID: templateID, @@ -89,8 +89,21 @@ func (s *StoreEnqueuer) buildPayload(ctx context.Context, userID uuid.UUID, temp return nil, xerrors.Errorf("new message metadata: %w", err) } + payload := types.MessagePayload{ + Version: "1.0", + + NotificationName: metadata.NotificationName, + + UserID: metadata.UserID.String(), + UserEmail: metadata.UserEmail, + UserName: metadata.UserName, + + Labels: labels, + // No actions yet + } + // Execute any templates in actions. - out, err := render.GoTemplate(string(metadata.Actions), types.MessagePayload{}, s.helpers) + out, err := render.GoTemplate(string(metadata.Actions), payload, s.helpers) if err != nil { return nil, xerrors.Errorf("render actions: %w", err) } @@ -100,19 +113,8 @@ func (s *StoreEnqueuer) buildPayload(ctx context.Context, userID uuid.UUID, temp if err = json.Unmarshal(metadata.Actions, &actions); err != nil { return nil, xerrors.Errorf("new message metadata: parse template actions: %w", err) } - - return &types.MessagePayload{ - Version: "1.0", - - NotificationName: metadata.NotificationName, - - UserID: metadata.UserID.String(), - UserEmail: metadata.UserEmail, - UserName: metadata.UserName, - - Actions: actions, - Labels: labels, - }, nil + payload.Actions = actions + return &payload, nil } // NoopEnqueuer implements the Enqueuer interface but performs a noop. diff --git a/coderd/notifications/render/gotmpl_test.go b/coderd/notifications/render/gotmpl_test.go index 32970dd6cd8b6..0cb95bccfcb43 100644 --- a/coderd/notifications/render/gotmpl_test.go +++ b/coderd/notifications/render/gotmpl_test.go @@ -38,6 +38,23 @@ func TestGoTemplate(t *testing.T) { expectedOutput: userEmail, expectedErr: nil, }, + { + name: "render workspace URL", + in: `[{ + "label": "View workspace", + "url": "{{ base_url }}/@{{.UserName}}/{{.Labels.name}}" + }]`, + payload: types.MessagePayload{ + UserName: "johndoe", + Labels: map[string]string{ + "name": "my-workspace", + }, + }, + expectedOutput: `[{ + "label": "View workspace", + "url": "https://mocked-server-address/@johndoe/my-workspace" + }]`, + }, } for _, tc := range tests { @@ -46,7 +63,9 @@ func TestGoTemplate(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - out, err := render.GoTemplate(tc.in, tc.payload, nil) + out, err := render.GoTemplate(tc.in, tc.payload, map[string]any{ + "base_url": func() string { return "https://mocked-server-address" }, + }) if tc.expectedErr == nil { require.NoError(t, err) } else { diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index 915a3f2042d55..8609d8a8cc170 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -1687,7 +1687,7 @@ func TestNotifications(t *testing.T) { require.Contains(t, notifEnq.sent[0].targets, workspace.OrganizationID) require.Contains(t, notifEnq.sent[0].targets, user.ID) if tc.deletionReason == database.BuildReasonInitiator { - require.Equal(t, notifEnq.sent[0].labels["initiator"], initiator.Username) + require.Equal(t, initiator.Username, notifEnq.sent[0].labels["initiator"]) } } else { require.Len(t, notifEnq.sent, 0) @@ -1798,8 +1798,8 @@ func TestNotifications(t *testing.T) { require.Contains(t, notifEnq.sent[0].targets, workspace.ID) require.Contains(t, notifEnq.sent[0].targets, workspace.OrganizationID) require.Contains(t, notifEnq.sent[0].targets, user.ID) - require.Equal(t, notifEnq.sent[0].labels["initiator"], "autobuild") - require.Equal(t, notifEnq.sent[0].labels["reason"], string(tc.buildReason)) + require.Equal(t, "autobuild", notifEnq.sent[0].labels["initiator"]) + require.Equal(t, string(tc.buildReason), notifEnq.sent[0].labels["reason"]) } else { require.Len(t, notifEnq.sent, 0) } From 3dd09d22f69f6b00e2e49faf5f09277168bbd4b4 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Tue, 16 Jul 2024 10:39:16 +0200 Subject: [PATCH 13/13] One more test --- coderd/notifications/manager_test.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/coderd/notifications/manager_test.go b/coderd/notifications/manager_test.go index abf29ca7a4539..93ba158b48a65 100644 --- a/coderd/notifications/manager_test.go +++ b/coderd/notifications/manager_test.go @@ -98,10 +98,11 @@ func TestBuildPayload(t *testing.T) { // GIVEN: a set of helpers to be injected into the templates const label = "Click here!" - const url = "http://xyz.com/" + const baseURL = "http://xyz.com" + const url = baseURL + "/@bobby/my-workspace" helpers := map[string]any{ "my_label": func() string { return label }, - "my_url": func() string { return url }, + "my_url": func() string { return baseURL }, } // GIVEN: an enqueue interceptor which returns mock metadata @@ -112,7 +113,7 @@ func TestBuildPayload(t *testing.T) { actions := []types.TemplateAction{ { Label: "{{ my_label }}", - URL: "{{ my_url }}", + URL: "{{ my_url }}/@{{.UserName}}/{{.Labels.name}}", }, } out, err := json.Marshal(actions) @@ -131,7 +132,9 @@ func TestBuildPayload(t *testing.T) { require.NoError(t, err) // WHEN: a notification is enqueued - _, err = enq.Enqueue(ctx, uuid.New(), notifications.TemplateWorkspaceDeleted, nil, "test") + _, err = enq.Enqueue(ctx, uuid.New(), notifications.TemplateWorkspaceDeleted, map[string]string{ + "name": "my-workspace", + }, "test") require.NoError(t, err) // THEN: expect that a payload will be constructed and have the expected values