From f673b57bea27d3df5ad472a96b122b9a0afe347e Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Tue, 13 Aug 2024 18:51:42 +0000 Subject: [PATCH 1/6] feat: notify template owner on manual build failures --- ...ons_template_manual_build_failure.down.sql | 0 ...tions_template_manual_build_failure.up.sql | 26 +++ coderd/notifications/events.go | 5 + .../provisionerdserver/provisionerdserver.go | 84 ++++++++- .../provisionerdserver_test.go | 170 +++++++++++++++++- 5 files changed, 272 insertions(+), 13 deletions(-) create mode 100644 coderd/database/migrations/000243_notifications_template_manual_build_failure.down.sql create mode 100644 coderd/database/migrations/000243_notifications_template_manual_build_failure.up.sql diff --git a/coderd/database/migrations/000243_notifications_template_manual_build_failure.down.sql b/coderd/database/migrations/000243_notifications_template_manual_build_failure.down.sql new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/coderd/database/migrations/000243_notifications_template_manual_build_failure.up.sql b/coderd/database/migrations/000243_notifications_template_manual_build_failure.up.sql new file mode 100644 index 0000000000000..5438cb808b074 --- /dev/null +++ b/coderd/database/migrations/000243_notifications_template_manual_build_failure.up.sql @@ -0,0 +1,26 @@ +INSERT INTO + notification_templates ( + id, + name, + title_template, + body_template, + "group", + actions + ) +VALUES ( + '48a9d2b9-3655-430c-a31a-2442479e7519', + 'Template Manual Build Failure', + E'Workspace with template "{{.Labels.name}}" failed to build', + E'Hi {{.UserName}}\n\nThe workspace **{{.Labels.workspaceName}}**, using the template **{{.Labels.name}}**, failed during a manual build({{.Labels.transition}}) initiated by the user **{{.Labels.initiator}}**.', + 'Template Events', + '[ + { + "label": "View build", + "url": "{{ base_url }}/@{{.Labels.workspaceUserName}}/{{.Labels.workspaceName}}/builds/{{.Labels.buildNumber}}" + }, + { + "label": "View Template", + "url": "{{ base_url }}/templates/{{.Labels.name}}" + } + ]'::jsonb + ); diff --git a/coderd/notifications/events.go b/coderd/notifications/events.go index 94260317e20c9..368e325f9f24d 100644 --- a/coderd/notifications/events.go +++ b/coderd/notifications/events.go @@ -19,3 +19,8 @@ var ( TemplateUserAccountCreated = uuid.MustParse("4e19c0ac-94e1-4532-9515-d1801aa283b2") TemplateUserAccountDeleted = uuid.MustParse("f44d9314-ad03-4bc8-95d0-5cad491da6b6") ) + +// Template-related events. +var ( + TemplateTemplateManualBuildFailed = uuid.MustParse("48a9d2b9-3655-430c-a31a-2442479e7519") +) diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 458f79ca348e6..2cd38f8bcae22 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -1020,7 +1020,25 @@ func (s *server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*proto. return nil, err } - s.notifyWorkspaceBuildFailed(ctx, workspace, build) + manualBuild := build.Reason.Valid() && build.Reason == database.BuildReasonInitiator + if !manualBuild { + s.notifyWorkspaceAutoBuildFailed(ctx, workspace, build) + } else { + template, err := s.Database.GetTemplateByID(ctx, workspace.TemplateID) + if err != nil { + return nil, xerrors.Errorf("get template to notify manual build failed: %w", err) + } + + owner, err := s.Database.GetUserByID(ctx, workspace.OwnerID) + if err != nil { + return nil, xerrors.Errorf("get owner to notify manual build failed: %w", err) + } + + // Only notify the template creator if the build was initiated by someone else + if build.InitiatorID != template.CreatedBy { + s.notifyTemplateManualBuildFailed(ctx, workspace, owner, template, build) + } + } err = s.Pubsub.Publish(codersdk.WorkspaceNotifyChannel(build.WorkspaceID), []byte{}) if err != nil { @@ -1095,13 +1113,8 @@ 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 build initiated by a user should not notify - } - reason = string(build.Reason) - +func (s *server) notifyWorkspaceAutoBuildFailed(ctx context.Context, workspace database.Workspace, build database.WorkspaceBuild) { + reason := string(build.Reason) if _, err := s.NotificationsEnqueuer.Enqueue(ctx, workspace.OwnerID, notifications.TemplateWorkspaceAutobuildFailed, map[string]string{ "name": workspace.Name, @@ -1110,7 +1123,24 @@ func (s *server) notifyWorkspaceBuildFailed(ctx context.Context, workspace datab // 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)) + s.Logger.Warn(ctx, "failed to notify of failed workspace autobuild", slog.F("workspace_id", workspace.ID), slog.Error(err)) + } +} + +func (s *server) notifyTemplateManualBuildFailed(ctx context.Context, workspace database.Workspace, owner database.User, template database.Template, build database.WorkspaceBuild) { + if _, err := s.NotificationsEnqueuer.Enqueue(ctx, template.CreatedBy, notifications.TemplateTemplateManualBuildFailed, + map[string]string{ + "name": template.Name, + "workspaceName": workspace.Name, + "transition": string(build.Transition), + "initiator": build.InitiatorByUsername, + "workspaceUserName": owner.Username, + "buildNumber": strconv.FormatInt(int64(build.BuildNumber), 10), + }, "provisionerdserver", + // Associate this notification with all the related entities. + workspace.ID, workspace.OwnerID, workspace.TemplateID, workspace.OrganizationID, template.CreatedBy, + ); err != nil { + s.Logger.Warn(ctx, "failed to notify of failed template autobuild", slog.F("template_id", template.ID), slog.F("workspace_id", workspace.ID), slog.Error(err)) } } @@ -1585,6 +1615,42 @@ func (s *server) notifyWorkspaceDeleted(ctx context.Context, workspace database. } } +func (s *server) notifyTemplateOwnerAboutManualBuildFailure(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: + if build.InitiatorID == workspace.OwnerID { + // Deletions initiated by self should not notify. + return + } + + reason = "initiated by user" + case database.BuildReasonAutodelete: + reason = "autodeleted due to dormancy" + initiator = "autobuild" + default: + reason = string(build.Reason) + } + } else { + reason = string(build.Reason) + s.Logger.Warn(ctx, "invalid build reason when sending deletion notification", + slog.F("reason", reason), slog.F("workspace_id", workspace.ID), slog.F("build_id", build.ID)) + } + + if _, err := s.NotificationsEnqueuer.Enqueue(ctx, workspace.OwnerID, notifications.TemplateWorkspaceDeleted, + 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 { + s.Logger.Warn(ctx, "failed to notify of workspace deletion", slog.Error(err)) + } +} func (s *server) startTrace(ctx context.Context, name string, opts ...trace.SpanStartOption) (context.Context, trace.Span) { return s.Tracer.Start(ctx, name, append(opts, trace.WithAttributes( semconv.ServiceNameKey.String("coderd.provisionerd"), diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index 79c1b00ac78ee..b7d21f6d30d0c 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -1568,7 +1568,7 @@ func TestInsertWorkspaceResource(t *testing.T) { func TestNotifications(t *testing.T) { t.Parallel() - t.Run("Workspace deletion", func(t *testing.T) { + t.Run("WorkspaceDeletion", func(t *testing.T) { t.Parallel() tests := []struct { @@ -1695,7 +1695,7 @@ func TestNotifications(t *testing.T) { } }) - t.Run("Workspace build failed", func(t *testing.T) { + t.Run("WorkspaceAutoBuildFailed", func(t *testing.T) { t.Parallel() tests := []struct { @@ -1705,12 +1705,12 @@ func TestNotifications(t *testing.T) { shouldNotify bool }{ { - name: "initiated by owner", + name: "InitiatedByOwner", buildReason: database.BuildReasonInitiator, shouldNotify: false, }, { - name: "initiated by autostart", + name: "InitiatedByAutoStart", buildReason: database.BuildReasonAutostart, shouldNotify: true, }, @@ -1804,6 +1804,168 @@ func TestNotifications(t *testing.T) { }) } }) + + t.Run("TemplateManualBuildFailed", func(t *testing.T) { + t.Parallel() + + t.Run("NotifyTemplateOwner", func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + notifEnq := &testutil.FakeNotificationsEnqueuer{} + + // Otherwise `(*Server).FailJob` fails with: + // audit log - get build {"error": "sql: no rows in result set"} + ignoreLogErrors := true + srv, db, ps, pd := setup(t, ignoreLogErrors, &overrides{ + notificationEnqueuer: notifEnq, + }) + + initiator := dbgen.User(t, db, database.User{}) + templateOwner := dbgen.User(t, db, database.User{}) + + template := dbgen.Template(t, db, database.Template{ + Name: "template", + Provisioner: database.ProvisionerTypeEcho, + OrganizationID: pd.OrganizationID, + CreatedBy: templateOwner.ID, + }) + template, err := db.GetTemplateByID(ctx, template.ID) + require.NoError(t, err) + file := dbgen.File(t, db, database.File{CreatedBy: initiator.ID}) + workspace := dbgen.Workspace(t, db, database.Workspace{ + TemplateID: template.ID, + OwnerID: initiator.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: database.BuildReasonInitiator, + }) + 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.FailJob(ctx, &proto.FailedJob{ + JobId: job.ID.String(), + Type: &proto.FailedJob_WorkspaceBuild_{ + WorkspaceBuild: &proto.FailedJob_WorkspaceBuild{ + State: []byte{}, + }, + }, + }) + require.NoError(t, err) + + require.Len(t, notifEnq.Sent, 1) + require.Equal(t, notifEnq.Sent[0].UserID, templateOwner.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, templateOwner.ID) + require.Contains(t, notifEnq.Sent[0].Targets, initiator.ID) + }) + + t.Run("DoNotNotifyTheInitiator", func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + notifEnq := &testutil.FakeNotificationsEnqueuer{} + + // Otherwise `(*Server).FailJob` fails with: + // audit log - get build {"error": "sql: no rows in result set"} + ignoreLogErrors := true + srv, db, ps, pd := setup(t, ignoreLogErrors, &overrides{ + notificationEnqueuer: notifEnq, + }) + + initiator := dbgen.User(t, db, database.User{}) + templateOwner := initiator + + template := dbgen.Template(t, db, database.Template{ + Name: "template", + Provisioner: database.ProvisionerTypeEcho, + OrganizationID: pd.OrganizationID, + CreatedBy: templateOwner.ID, + }) + template, err := db.GetTemplateByID(ctx, template.ID) + require.NoError(t, err) + file := dbgen.File(t, db, database.File{CreatedBy: initiator.ID}) + workspace := dbgen.Workspace(t, db, database.Workspace{ + TemplateID: template.ID, + OwnerID: initiator.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: database.BuildReasonInitiator, + }) + 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.FailJob(ctx, &proto.FailedJob{ + JobId: job.ID.String(), + Type: &proto.FailedJob_WorkspaceBuild_{ + WorkspaceBuild: &proto.FailedJob_WorkspaceBuild{ + State: []byte{}, + }, + }, + }) + require.NoError(t, err) + + require.Len(t, notifEnq.Sent, 0) + }) + }) } type overrides struct { From cdf247b5b1a5a1c4e6a6cd9723a920c5ff0e6c5b Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Wed, 14 Aug 2024 17:59:51 +0000 Subject: [PATCH 2/6] Apply dannys suggestions --- ...ns_template_manual_build_failure.down.sql} | 0 ...ions_template_manual_build_failure.up.sql} | 2 +- .../provisionerdserver/provisionerdserver.go | 38 +-- .../provisionerdserver_test.go | 252 +++++++----------- 4 files changed, 104 insertions(+), 188 deletions(-) rename coderd/database/migrations/{000243_notifications_template_manual_build_failure.down.sql => 000245_notifications_template_manual_build_failure.down.sql} (100%) rename coderd/database/migrations/{000243_notifications_template_manual_build_failure.up.sql => 000245_notifications_template_manual_build_failure.up.sql} (95%) diff --git a/coderd/database/migrations/000243_notifications_template_manual_build_failure.down.sql b/coderd/database/migrations/000245_notifications_template_manual_build_failure.down.sql similarity index 100% rename from coderd/database/migrations/000243_notifications_template_manual_build_failure.down.sql rename to coderd/database/migrations/000245_notifications_template_manual_build_failure.down.sql diff --git a/coderd/database/migrations/000243_notifications_template_manual_build_failure.up.sql b/coderd/database/migrations/000245_notifications_template_manual_build_failure.up.sql similarity index 95% rename from coderd/database/migrations/000243_notifications_template_manual_build_failure.up.sql rename to coderd/database/migrations/000245_notifications_template_manual_build_failure.up.sql index 5438cb808b074..885c8899e085a 100644 --- a/coderd/database/migrations/000243_notifications_template_manual_build_failure.up.sql +++ b/coderd/database/migrations/000245_notifications_template_manual_build_failure.up.sql @@ -19,7 +19,7 @@ VALUES ( "url": "{{ base_url }}/@{{.Labels.workspaceUserName}}/{{.Labels.workspaceName}}/builds/{{.Labels.buildNumber}}" }, { - "label": "View Template", + "label": "View template", "url": "{{ base_url }}/templates/{{.Labels.name}}" } ]'::jsonb diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 2cd38f8bcae22..b51a53b7107cf 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -1140,7 +1140,7 @@ func (s *server) notifyTemplateManualBuildFailed(ctx context.Context, workspace // Associate this notification with all the related entities. workspace.ID, workspace.OwnerID, workspace.TemplateID, workspace.OrganizationID, template.CreatedBy, ); err != nil { - s.Logger.Warn(ctx, "failed to notify of failed template autobuild", slog.F("template_id", template.ID), slog.F("workspace_id", workspace.ID), slog.Error(err)) + s.Logger.Warn(ctx, "failed to notify of failed template manual build", slog.F("template_id", template.ID), slog.F("workspace_id", workspace.ID), slog.Error(err)) } } @@ -1615,42 +1615,6 @@ func (s *server) notifyWorkspaceDeleted(ctx context.Context, workspace database. } } -func (s *server) notifyTemplateOwnerAboutManualBuildFailure(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: - if build.InitiatorID == workspace.OwnerID { - // Deletions initiated by self should not notify. - return - } - - reason = "initiated by user" - case database.BuildReasonAutodelete: - reason = "autodeleted due to dormancy" - initiator = "autobuild" - default: - reason = string(build.Reason) - } - } else { - reason = string(build.Reason) - s.Logger.Warn(ctx, "invalid build reason when sending deletion notification", - slog.F("reason", reason), slog.F("workspace_id", workspace.ID), slog.F("build_id", build.ID)) - } - - if _, err := s.NotificationsEnqueuer.Enqueue(ctx, workspace.OwnerID, notifications.TemplateWorkspaceDeleted, - 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 { - s.Logger.Warn(ctx, "failed to notify of workspace deletion", slog.Error(err)) - } -} func (s *server) startTrace(ctx context.Context, name string, opts ...trace.SpanStartOption) (context.Context, trace.Span) { return s.Tracer.Start(ctx, name, append(opts, trace.WithAttributes( semconv.ServiceNameKey.String("coderd.provisionerd"), diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index b7d21f6d30d0c..79fae0f9cb02c 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -1699,8 +1699,7 @@ func TestNotifications(t *testing.T) { t.Parallel() tests := []struct { - name string - + name string buildReason database.BuildReason shouldNotify bool }{ @@ -1808,163 +1807,116 @@ func TestNotifications(t *testing.T) { t.Run("TemplateManualBuildFailed", func(t *testing.T) { t.Parallel() - t.Run("NotifyTemplateOwner", func(t *testing.T) { - t.Parallel() - - ctx := context.Background() - notifEnq := &testutil.FakeNotificationsEnqueuer{} - - // Otherwise `(*Server).FailJob` fails with: - // audit log - get build {"error": "sql: no rows in result set"} - ignoreLogErrors := true - srv, db, ps, pd := setup(t, ignoreLogErrors, &overrides{ + 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, }) + userA = dbgen.User(t, db, database.User{}) + userB = dbgen.User(t, db, database.User{}) + ) - initiator := dbgen.User(t, db, database.User{}) - templateOwner := dbgen.User(t, db, database.User{}) - - template := dbgen.Template(t, db, database.Template{ - Name: "template", - Provisioner: database.ProvisionerTypeEcho, - OrganizationID: pd.OrganizationID, - CreatedBy: templateOwner.ID, - }) - template, err := db.GetTemplateByID(ctx, template.ID) - require.NoError(t, err) - file := dbgen.File(t, db, database.File{CreatedBy: initiator.ID}) - workspace := dbgen.Workspace(t, db, database.Workspace{ - TemplateID: template.ID, - OwnerID: initiator.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: database.BuildReasonInitiator, - }) - 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) + tc := []struct { + name string + owner database.User + initiator database.User + shouldNotify bool + }{ + { + name: "InitiatedByOwner", + owner: userA, + initiator: userA, + shouldNotify: false, + }, + { + name: "InitiatedBySomeoneElse", + owner: userB, + initiator: userA, + shouldNotify: true, + }, + } - _, err = srv.FailJob(ctx, &proto.FailedJob{ - JobId: job.ID.String(), - Type: &proto.FailedJob_WorkspaceBuild_{ - WorkspaceBuild: &proto.FailedJob_WorkspaceBuild{ - State: []byte{}, + for _, c := range tc { + t.Run(c.name, func(t *testing.T) { + // Given: a template created by the owner + template := dbgen.Template(t, db, database.Template{ + Name: "template", + Provisioner: database.ProvisionerTypeEcho, + OrganizationID: pd.OrganizationID, + CreatedBy: c.owner.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, }, - }, - }) - require.NoError(t, err) - - require.Len(t, notifEnq.Sent, 1) - require.Equal(t, notifEnq.Sent[0].UserID, templateOwner.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, templateOwner.ID) - require.Contains(t, notifEnq.Sent[0].Targets, initiator.ID) - }) - - t.Run("DoNotNotifyTheInitiator", func(t *testing.T) { - t.Parallel() - - ctx := context.Background() - notifEnq := &testutil.FakeNotificationsEnqueuer{} - - // Otherwise `(*Server).FailJob` fails with: - // audit log - get build {"error": "sql: no rows in result set"} - ignoreLogErrors := true - srv, db, ps, pd := setup(t, ignoreLogErrors, &overrides{ - notificationEnqueuer: notifEnq, - }) - - initiator := dbgen.User(t, db, database.User{}) - templateOwner := initiator + JobID: uuid.New(), + }) - template := dbgen.Template(t, db, database.Template{ - Name: "template", - Provisioner: database.ProvisionerTypeEcho, - OrganizationID: pd.OrganizationID, - CreatedBy: templateOwner.ID, - }) - template, err := db.GetTemplateByID(ctx, template.ID) - require.NoError(t, err) - file := dbgen.File(t, db, database.File{CreatedBy: initiator.ID}) - workspace := dbgen.Workspace(t, db, database.Workspace{ - TemplateID: template.ID, - OwnerID: initiator.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: database.BuildReasonInitiator, - }) - 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) + // And: a workspace build initiated manually by a user + workspace := dbgen.Workspace(t, db, database.Workspace{ + TemplateID: template.ID, + OwnerID: c.initiator.ID, + OrganizationID: pd.OrganizationID, + }) + build := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ + WorkspaceID: workspace.ID, + TemplateVersionID: version.ID, + InitiatorID: c.initiator.ID, + Transition: database.WorkspaceTransitionDelete, + Reason: database.BuildReasonInitiator, + }) + file := dbgen.File(t, db, database.File{CreatedBy: c.initiator.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) - _, err = srv.FailJob(ctx, &proto.FailedJob{ - JobId: job.ID.String(), - Type: &proto.FailedJob_WorkspaceBuild_{ - WorkspaceBuild: &proto.FailedJob_WorkspaceBuild{ - State: []byte{}, + // When: the workspace build job 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) + }) + require.NoError(t, err) - require.Len(t, notifEnq.Sent, 0) - }) + // Then: send the appropriate notifications + if c.shouldNotify { + require.Len(t, notifEnq.Sent, 1) + require.Equal(t, notifEnq.Sent[0].UserID, c.owner.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, c.owner.ID) + require.Contains(t, notifEnq.Sent[0].Targets, c.initiator.ID) + } else { + require.Len(t, notifEnq.Sent, 0) + } + }) + } }) } From 9c36cb8a3e13d451d2fc62094e566eac22a38422 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Thu, 15 Aug 2024 14:25:56 +0000 Subject: [PATCH 3/6] Rollback notify changes --- .../provisionerdserver/provisionerdserver.go | 48 ++++--------------- 1 file changed, 9 insertions(+), 39 deletions(-) diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index b51a53b7107cf..458f79ca348e6 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -1020,25 +1020,7 @@ func (s *server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*proto. return nil, err } - manualBuild := build.Reason.Valid() && build.Reason == database.BuildReasonInitiator - if !manualBuild { - s.notifyWorkspaceAutoBuildFailed(ctx, workspace, build) - } else { - template, err := s.Database.GetTemplateByID(ctx, workspace.TemplateID) - if err != nil { - return nil, xerrors.Errorf("get template to notify manual build failed: %w", err) - } - - owner, err := s.Database.GetUserByID(ctx, workspace.OwnerID) - if err != nil { - return nil, xerrors.Errorf("get owner to notify manual build failed: %w", err) - } - - // Only notify the template creator if the build was initiated by someone else - if build.InitiatorID != template.CreatedBy { - s.notifyTemplateManualBuildFailed(ctx, workspace, owner, template, build) - } - } + s.notifyWorkspaceBuildFailed(ctx, workspace, build) err = s.Pubsub.Publish(codersdk.WorkspaceNotifyChannel(build.WorkspaceID), []byte{}) if err != nil { @@ -1113,8 +1095,13 @@ func (s *server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*proto. return &proto.Empty{}, nil } -func (s *server) notifyWorkspaceAutoBuildFailed(ctx context.Context, workspace database.Workspace, build database.WorkspaceBuild) { - reason := string(build.Reason) +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 build initiated by a user should not notify + } + reason = string(build.Reason) + if _, err := s.NotificationsEnqueuer.Enqueue(ctx, workspace.OwnerID, notifications.TemplateWorkspaceAutobuildFailed, map[string]string{ "name": workspace.Name, @@ -1123,24 +1110,7 @@ func (s *server) notifyWorkspaceAutoBuildFailed(ctx context.Context, workspace d // 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.F("workspace_id", workspace.ID), slog.Error(err)) - } -} - -func (s *server) notifyTemplateManualBuildFailed(ctx context.Context, workspace database.Workspace, owner database.User, template database.Template, build database.WorkspaceBuild) { - if _, err := s.NotificationsEnqueuer.Enqueue(ctx, template.CreatedBy, notifications.TemplateTemplateManualBuildFailed, - map[string]string{ - "name": template.Name, - "workspaceName": workspace.Name, - "transition": string(build.Transition), - "initiator": build.InitiatorByUsername, - "workspaceUserName": owner.Username, - "buildNumber": strconv.FormatInt(int64(build.BuildNumber), 10), - }, "provisionerdserver", - // Associate this notification with all the related entities. - workspace.ID, workspace.OwnerID, workspace.TemplateID, workspace.OrganizationID, template.CreatedBy, - ); err != nil { - s.Logger.Warn(ctx, "failed to notify of failed template manual build", slog.F("template_id", template.ID), slog.F("workspace_id", workspace.ID), slog.Error(err)) + s.Logger.Warn(ctx, "failed to notify of failed workspace autobuild", slog.Error(err)) } } From f81611df82f6a4c4d34cf8f47ca8e22651f83c59 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Fri, 16 Aug 2024 15:03:30 +0000 Subject: [PATCH 4/6] Fix tests and notifications --- ...fications_template_build_failure.down.sql} | 0 ...otifications_template_build_failure.up.sql | 33 ++++++ ...tions_template_manual_build_failure.up.sql | 26 ----- coderd/notifications/events.go | 4 +- .../provisionerdserver/provisionerdserver.go | 90 ++++++++++++-- .../provisionerdserver_test.go | 110 ++++++++++++------ 6 files changed, 187 insertions(+), 76 deletions(-) rename coderd/database/migrations/{000245_notifications_template_manual_build_failure.down.sql => 000245_notifications_template_build_failure.down.sql} (100%) create mode 100644 coderd/database/migrations/000245_notifications_template_build_failure.up.sql delete mode 100644 coderd/database/migrations/000245_notifications_template_manual_build_failure.up.sql diff --git a/coderd/database/migrations/000245_notifications_template_manual_build_failure.down.sql b/coderd/database/migrations/000245_notifications_template_build_failure.down.sql similarity index 100% rename from coderd/database/migrations/000245_notifications_template_manual_build_failure.down.sql rename to coderd/database/migrations/000245_notifications_template_build_failure.down.sql diff --git a/coderd/database/migrations/000245_notifications_template_build_failure.up.sql b/coderd/database/migrations/000245_notifications_template_build_failure.up.sql new file mode 100644 index 0000000000000..772e983897fa7 --- /dev/null +++ b/coderd/database/migrations/000245_notifications_template_build_failure.up.sql @@ -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 + ); diff --git a/coderd/database/migrations/000245_notifications_template_manual_build_failure.up.sql b/coderd/database/migrations/000245_notifications_template_manual_build_failure.up.sql deleted file mode 100644 index 885c8899e085a..0000000000000 --- a/coderd/database/migrations/000245_notifications_template_manual_build_failure.up.sql +++ /dev/null @@ -1,26 +0,0 @@ -INSERT INTO - notification_templates ( - id, - name, - title_template, - body_template, - "group", - actions - ) -VALUES ( - '48a9d2b9-3655-430c-a31a-2442479e7519', - 'Template Manual Build Failure', - E'Workspace with template "{{.Labels.name}}" failed to build', - E'Hi {{.UserName}}\n\nThe workspace **{{.Labels.workspaceName}}**, using the template **{{.Labels.name}}**, failed during a manual build({{.Labels.transition}}) initiated by the user **{{.Labels.initiator}}**.', - 'Template Events', - '[ - { - "label": "View build", - "url": "{{ base_url }}/@{{.Labels.workspaceUserName}}/{{.Labels.workspaceName}}/builds/{{.Labels.buildNumber}}" - }, - { - "label": "View template", - "url": "{{ base_url }}/templates/{{.Labels.name}}" - } - ]'::jsonb - ); diff --git a/coderd/notifications/events.go b/coderd/notifications/events.go index 785eab9595060..055596c62196d 100644 --- a/coderd/notifications/events.go +++ b/coderd/notifications/events.go @@ -22,6 +22,6 @@ var ( // Template-related events. var ( - TemplateTemplateManualBuildFailed = uuid.MustParse("48a9d2b9-3655-430c-a31a-2442479e7519") - TemplateTemplateDeleted = uuid.MustParse("29a09665-2a4c-403f-9648-54301670e7be") + TemplateTemplateBuildFailed = uuid.MustParse("48a9d2b9-3655-430c-a31a-2442479e7519") + TemplateTemplateDeleted = uuid.MustParse("29a09665-2a4c-403f-9648-54301670e7be") ) diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 458f79ca348e6..28e060226d85e 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -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 + autoBuild := build.Reason.Valid() && build.Reason != database.BuildReasonInitiator + if autoBuild { + s.notifyWorkspaceBuildFailed(ctx, workspace, build, workspace.OwnerID) + } + + // Notify template admins including owners + admins, err := findTemplateAdmins(ctx, s.Database) + if err != nil { + 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 { + s.notifyTemplateBuildFailed(ctx, workspace, build, admin.ID) + } + } + } err = s.Pubsub.Publish(codersdk.WorkspaceNotifyChannel(build.WorkspaceID), []byte{}) if err != nil { @@ -1095,17 +1111,11 @@ 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 build initiated by a user should not notify - } - 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) { + if _, err := s.NotificationsEnqueuer.Enqueue(ctx, receiverID, notifications.TemplateWorkspaceAutobuildFailed, map[string]string{ "name": workspace.Name, - "reason": reason, + "reason": string(build.Reason), }, "provisionerdserver", // Associate this notification with all the related entities. workspace.ID, workspace.OwnerID, workspace.TemplateID, workspace.OrganizationID, @@ -1114,6 +1124,49 @@ func (s *server) notifyWorkspaceBuildFailed(ctx context.Context, workspace datab } } +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)) + return + } + + owner, err := s.Database.GetUserByID(ctx, workspace.OwnerID) + if err != nil { + s.Logger.Warn(ctx, "failed to get workspace owner", slog.Error(err)) + return + } + + version, err := s.Database.GetTemplateVersionByID(ctx, build.TemplateVersionID) + if err != nil { + s.Logger.Warn(ctx, "failed to get template version", slog.Error(err)) + return + } + + // We only need to know the reason when it is not initiated by the user. + var reason string + if build.Reason != database.BuildReasonInitiator { + reason = string(build.Reason) + } + + if _, err := s.NotificationsEnqueuer.Enqueue(ctx, receiverID, notifications.TemplateTemplateBuildFailed, + map[string]string{ + "name": template.Name, + "version": version.Name, + "workspaceName": workspace.Name, + "transition": string(build.Transition), + "reason": reason, + "initiator": build.InitiatorByUsername, + "workspaceOwnerName": owner.Username, + "buildNumber": strconv.FormatInt(int64(build.BuildNumber), 10), + }, "provisionerdserver", + // Associate this notification with all the related entities. + workspace.ID, workspace.OwnerID, workspace.TemplateID, workspace.OrganizationID, template.CreatedBy, + ); err != nil { + s.Logger.Warn(ctx, "failed to notify of failed template manual build", slog.F("template_id", template.ID), slog.F("workspace_id", workspace.ID), 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()) @@ -2100,3 +2153,20 @@ func convertDisplayApps(apps *sdkproto.DisplayApps) []database.DisplayApp { } return dapps } + +// findTemplateAdmins fetches all users with template admin permission including owners. +func findTemplateAdmins(ctx context.Context, store database.Store) ([]database.GetUsersRow, error) { + owners, err := store.GetUsers(ctx, database.GetUsersParams{ + RbacRole: []string{codersdk.RoleOwner}, + }) + if err != nil { + return nil, xerrors.Errorf("get owners: %w", err) + } + templateAdmins, err := store.GetUsers(ctx, database.GetUsersParams{ + RbacRole: []string{codersdk.RoleTemplateAdmin}, + }) + if err != nil { + return nil, xerrors.Errorf("get template admins: %w", err) + } + return append(owners, templateAdmins...), nil +} diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index 79fae0f9cb02c..7cbbb27fcb9ba 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "io" "net/url" + "strconv" "strings" "sync" "sync/atomic" @@ -1804,7 +1805,7 @@ func TestNotifications(t *testing.T) { } }) - t.Run("TemplateManualBuildFailed", func(t *testing.T) { + t.Run("TemplateBuildFailed", func(t *testing.T) { t.Parallel() var ( @@ -1816,38 +1817,55 @@ func TestNotifications(t *testing.T) { srv, db, ps, pd = setup(t, ignoreLogErrors, &overrides{ notificationEnqueuer: notifEnq, }) - userA = dbgen.User(t, db, database.User{}) - userB = dbgen.User(t, db, database.User{}) + // 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 - owner database.User - initiator database.User - shouldNotify bool + name string + tplOwner database.User + buildInitiator *database.User + reason database.BuildReason + transition database.WorkspaceTransition + receivers []uuid.UUID }{ { - name: "InitiatedByOwner", - owner: userA, - initiator: userA, - shouldNotify: false, + 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: "InitiatedBySomeoneElse", - owner: userB, - initiator: userA, - shouldNotify: true, + name: "AutoBuild", + tplOwner: ownerA, + buildInitiator: nil, + 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 created by the owner + // Given: a template and a workspace build + isManualBuild := c.buildInitiator != nil && c.reason == database.BuildReasonInitiator template := dbgen.Template(t, db, database.Template{ Name: "template", Provisioner: database.ProvisionerTypeEcho, OrganizationID: pd.OrganizationID, - CreatedBy: c.owner.ID, + CreatedBy: c.tplOwner.ID, }) template, err := db.GetTemplateByID(ctx, template.ID) require.NoError(t, err) @@ -1859,21 +1877,23 @@ func TestNotifications(t *testing.T) { }, JobID: uuid.New(), }) - - // And: a workspace build initiated manually by a user + workspaceOwner := member workspace := dbgen.Workspace(t, db, database.Workspace{ TemplateID: template.ID, - OwnerID: c.initiator.ID, + OwnerID: workspaceOwner.ID, OrganizationID: pd.OrganizationID, }) - build := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ + build := database.WorkspaceBuild{ WorkspaceID: workspace.ID, TemplateVersionID: version.ID, - InitiatorID: c.initiator.ID, - Transition: database.WorkspaceTransitionDelete, - Reason: database.BuildReasonInitiator, - }) - file := dbgen.File(t, db, database.File{CreatedBy: c.initiator.ID}) + Transition: c.transition, + Reason: c.reason, + } + // Set the build initiator if the test case specifies one for manual builds. + if isManualBuild { + build.InitiatorID = c.buildInitiator.ID + } + 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, @@ -1892,7 +1912,7 @@ func TestNotifications(t *testing.T) { }) require.NoError(t, err) - // When: the workspace build job fails + // When: the build fails _, err = srv.FailJob(ctx, &proto.FailedJob{ JobId: job.ID.String(), Type: &proto.FailedJob_WorkspaceBuild_{ @@ -1903,17 +1923,31 @@ func TestNotifications(t *testing.T) { }) require.NoError(t, err) - // Then: send the appropriate notifications - if c.shouldNotify { - require.Len(t, notifEnq.Sent, 1) - require.Equal(t, notifEnq.Sent[0].UserID, c.owner.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, c.owner.ID) - require.Contains(t, notifEnq.Sent[0].Targets, c.initiator.ID) - } else { - require.Len(t, notifEnq.Sent, 0) + // Then: send the template build failed notifications + var buildFailedNotifications []*testutil.Notification + for _, n := range notifEnq.Sent { + if n.TemplateID == notifications.TemplateTemplateBuildFailed { + 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.Name) + require.Equal(t, n.Labels["buildNumber"], strconv.FormatInt(int64(build.BuildNumber), 10)) + if isManualBuild { + require.Equal(t, n.Labels["initiator"], c.buildInitiator.Username) + } } }) } From 35abdaeb74fcc2ef1aac625bf40a5207ab11276f Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Mon, 19 Aug 2024 19:15:07 +0000 Subject: [PATCH 5/6] Fix workspace build --- coderd/provisionerdserver/provisionerdserver_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index 7cbbb27fcb9ba..85931db7ce74d 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -1883,12 +1883,12 @@ func TestNotifications(t *testing.T) { OwnerID: workspaceOwner.ID, OrganizationID: pd.OrganizationID, }) - build := database.WorkspaceBuild{ + build := dbgen.WorkspaceBuild(t, db, 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 { build.InitiatorID = c.buildInitiator.ID From 204b17e0666de3b96d1bd7c734822f72d6777af1 Mon Sep 17 00:00:00 2001 From: BrunoQuaresma Date: Mon, 19 Aug 2024 19:54:24 +0000 Subject: [PATCH 6/6] Try to fix tests --- coderd/provisionerdserver/provisionerdserver.go | 12 +++--------- .../provisionerdserver_test.go | 17 +++++++++-------- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 0b8ce1b75102e..680876d9a88cf 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -1021,7 +1021,7 @@ func (s *server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*proto. } // Only notify auto build failures - autoBuild := build.Reason.Valid() && build.Reason != database.BuildReasonInitiator + autoBuild := build.Reason != database.BuildReasonInitiator if autoBuild { s.notifyWorkspaceBuildFailed(ctx, workspace, build, workspace.OwnerID) } @@ -1032,7 +1032,7 @@ func (s *server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*proto. 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 { + if admin.ID != build.InitiatorID && !autoBuild { s.notifyTemplateBuildFailed(ctx, workspace, build, admin.ID) } } @@ -1143,19 +1143,13 @@ func (s *server) notifyTemplateBuildFailed(ctx context.Context, workspace databa return } - // We only need to know the reason when it is not initiated by the user. - var reason string - if build.Reason != database.BuildReasonInitiator { - reason = string(build.Reason) - } - if _, err := s.NotificationsEnqueuer.Enqueue(ctx, receiverID, notifications.TemplateTemplateBuildFailed, map[string]string{ "name": template.Name, "version": version.Name, "workspaceName": workspace.Name, "transition": string(build.Transition), - "reason": reason, + "reason": string(build.Reason), "initiator": build.InitiatorByUsername, "workspaceOwnerName": owner.Username, "buildNumber": strconv.FormatInt(int64(build.BuildNumber), 10), diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index 85931db7ce74d..607a6764fc4dc 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -1829,7 +1829,7 @@ func TestNotifications(t *testing.T) { tc := []struct { name string tplOwner database.User - buildInitiator *database.User + buildInitiator database.User reason database.BuildReason transition database.WorkspaceTransition receivers []uuid.UUID @@ -1837,7 +1837,7 @@ func TestNotifications(t *testing.T) { { name: "ManualBuild", tplOwner: ownerA, - buildInitiator: &ownerB, + buildInitiator: ownerB, reason: database.BuildReasonInitiator, transition: database.WorkspaceTransitionStart, // Ensure that during manual builds, the initiator does not receive a @@ -1848,7 +1848,7 @@ func TestNotifications(t *testing.T) { { name: "AutoBuild", tplOwner: ownerA, - buildInitiator: nil, + buildInitiator: ownerA, reason: database.BuildReasonAutostart, transition: database.WorkspaceTransitionStart, // Ensure that during automated builds, all template admins and owners @@ -1860,7 +1860,7 @@ func TestNotifications(t *testing.T) { for _, c := range tc { t.Run(c.name, func(t *testing.T) { // Given: a template and a workspace build - isManualBuild := c.buildInitiator != nil && c.reason == database.BuildReasonInitiator + isManualBuild := c.reason == database.BuildReasonInitiator template := dbgen.Template(t, db, database.Template{ Name: "template", Provisioner: database.ProvisionerTypeEcho, @@ -1883,16 +1883,17 @@ func TestNotifications(t *testing.T) { OwnerID: workspaceOwner.ID, OrganizationID: pd.OrganizationID, }) - build := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ + 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 { - build.InitiatorID = c.buildInitiator.ID + 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, @@ -1943,7 +1944,7 @@ func TestNotifications(t *testing.T) { 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.Name) + 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)