Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit1d85b8c

Browse files
committed
review
1 parentbe9d7aa commit1d85b8c

File tree

13 files changed

+44
-41
lines changed

13 files changed

+44
-41
lines changed

‎coderd/agentapi/resources_monitoring.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,7 @@ func (a *ResourcesMonitoringAPI) monitorMemory(ctx context.Context, datapoints [
161161
workspace.OwnerID,
162162
workspace.OrganizationID,
163163
)
164-
iferr!=nil&&notifications.IsSeriousEnqueueError(err){
164+
iferr!=nil {
165165
returnxerrors.Errorf("notify workspace OOM: %w",err)
166166
}
167167

@@ -254,7 +254,7 @@ func (a *ResourcesMonitoringAPI) monitorVolumes(ctx context.Context, datapoints
254254
workspace.ID,
255255
workspace.OwnerID,
256256
workspace.OrganizationID,
257-
);err!=nil&&notifications.IsSeriousEnqueueError(err){
257+
);err!=nil {
258258
returnxerrors.Errorf("notify workspace OOD: %w",err)
259259
}
260260

‎coderd/autobuild/lifecycle_executor.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -442,7 +442,7 @@ func (e *Executor) runOnce(t time.Time) Stats {
442442
},"autobuild",
443443
// Associate this notification with all the related entities.
444444
ws.ID,ws.OwnerID,ws.TemplateID,ws.OrganizationID,
445-
);err!=nil&&notifications.IsSeriousEnqueueError(err){
445+
);err!=nil {
446446
log.Warn(e.ctx,"failed to notify of autoupdated workspace",slog.Error(err))
447447
}
448448
}
@@ -476,7 +476,7 @@ func (e *Executor) runOnce(t time.Time) Stats {
476476
ws.TemplateID,
477477
ws.OrganizationID,
478478
)
479-
iferr!=nil&&notifications.IsSeriousEnqueueError(err){
479+
iferr!=nil {
480480
log.Warn(e.ctx,"failed to notify of workspace marked as dormant",slog.Error(err),slog.F("workspace_id",ws.ID))
481481
}
482482
}

‎coderd/notifications/enqueuer.go‎

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -21,16 +21,7 @@ import (
2121
"github.com/coder/coder/v2/codersdk"
2222
)
2323

24-
var (
25-
ErrCannotEnqueueDisabledNotification=xerrors.New("notification is not enabled")
26-
ErrDuplicate=xerrors.New("duplicate notification")
27-
)
28-
29-
// IsSeriousEnqueueError filters out trivial errors which can be ignored by
30-
// most callers of Enqueue.
31-
funcIsSeriousEnqueueError(errerror)bool {
32-
return!xerrors.Is(err,ErrCannotEnqueueDisabledNotification)&&!xerrors.Is(err,ErrDuplicate)
33-
}
24+
varErrCannotEnqueueDisabledNotification=xerrors.New("notification is not enabled")
3425

3526
typeInvalidDefaultNotificationMethodErrorstruct {
3627
Methodstring
@@ -79,12 +70,14 @@ func NewStoreEnqueuer(cfg codersdk.NotificationsConfig, store Store, helpers tem
7970
}
8071

8172
// Enqueue queues a notification message for later delivery, assumes no structured input data.
73+
// Returns the IDs of successfully enqueued messages, if any.
8274
func (s*StoreEnqueuer)Enqueue(ctx context.Context,userID,templateID uuid.UUID,labelsmap[string]string,createdBystring,targets...uuid.UUID) ([]uuid.UUID,error) {
8375
returns.EnqueueWithData(ctx,userID,templateID,labels,nil,createdBy,targets...)
8476
}
8577

8678
// Enqueue queues a notification message for later delivery.
8779
// Messages will be dequeued by a notifier later and dispatched.
80+
// Returns the IDs of successfully enqueued messages, if any.
8881
func (s*StoreEnqueuer)EnqueueWithData(ctx context.Context,userID,templateID uuid.UUID,labelsmap[string]string,datamap[string]any,createdBystring,targets...uuid.UUID) ([]uuid.UUID,error) {
8982
metadata,err:=s.store.FetchNewMessageMetadata(ctx, database.FetchNewMessageMetadataParams{
9083
UserID:userID,
@@ -147,14 +140,16 @@ func (s *StoreEnqueuer) EnqueueWithData(ctx context.Context, userID, templateID
147140
//
148141
// This is more efficient than fetching the user's preferences for each enqueue, and centralizes the business logic.
149142
ifstrings.Contains(err.Error(),ErrCannotEnqueueDisabledNotification.Error()) {
150-
returnnil,ErrCannotEnqueueDisabledNotification
143+
s.log.Debug(ctx,"not enqueueing disabled notification",slog.F("template_id",templateID),slog.F("user_id",userID),slog.F("method",method))
144+
continue
151145
}
152146

153147
// If the enqueue fails due to a dedupe hash conflict, this means that a notification has already been enqueued
154148
// today with identical properties. It's far simpler to prevent duplicate sends in this central manner, rather than
155149
// having each notification enqueue handle its own logic.
156150
ifdatabase.IsUniqueViolation(err,database.UniqueNotificationMessagesDedupeHashIndex) {
157-
returnnil,ErrDuplicate
151+
s.log.Debug(ctx,"not enqueueing duplicate notification",slog.F("template_id",templateID),slog.F("user_id",userID),slog.F("method",method))
152+
continue
158153
}
159154

160155
s.log.Warn(ctx,"failed to enqueue notification",slog.F("template_id",templateID),slog.F("input",input),slog.Error(err))

‎coderd/notifications/notifications_test.go‎

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import (
3535
"golang.org/x/xerrors"
3636

3737
"cdr.dev/slog"
38+
"cdr.dev/slog/sloggers/sloghuman"
3839
"github.com/coder/quartz"
3940
"github.com/coder/serpent"
4041

@@ -1654,18 +1655,20 @@ func TestDisabledByDefaultBeforeEnqueue(t *testing.T) {
16541655

16551656
ctx:=dbauthz.AsNotifier(testutil.Context(t,testutil.WaitSuperLong))
16561657
store,_:=dbtestutil.NewDB(t)
1657-
logger:=testutil.Logger(t)
1658+
logbuf:= strings.Builder{}
1659+
logger:=testutil.Logger(t).AppendSinks(sloghuman.Sink(&logbuf)).Leveled(slog.LevelDebug)
16581660

16591661
cfg:=defaultNotificationsConfig(database.NotificationMethodSmtp)
16601662
enq,err:=notifications.NewStoreEnqueuer(cfg,store,defaultHelpers(),logger.Named("enqueuer"),quartz.NewReal())
16611663
require.NoError(t,err)
16621664
user:=createSampleUser(t,store)
16631665

16641666
// We want to try enqueuing a notification on a template that is disabled
1665-
// by default. We expect this tofail.
1667+
// by default. We expect this tobe a no-op that produces a debug log.
16661668
templateID:=notifications.TemplateWorkspaceManuallyUpdated
16671669
_,err=enq.Enqueue(ctx,user.ID,templateID,map[string]string{},"test")
1668-
require.ErrorIs(t,err,notifications.ErrCannotEnqueueDisabledNotification,"enqueuing did not fail with expected error")
1670+
require.NoError(t,err)
1671+
require.Contains(t,logbuf.String(),"not enqueueing disabled notification")
16691672
}
16701673

16711674
// TestDisabledBeforeEnqueue ensures that notifications cannot be enqueued once a user has disabled that notification template
@@ -1679,7 +1682,8 @@ func TestDisabledBeforeEnqueue(t *testing.T) {
16791682

16801683
ctx:=dbauthz.AsNotifier(testutil.Context(t,testutil.WaitSuperLong))
16811684
store,_:=dbtestutil.NewDB(t)
1682-
logger:=testutil.Logger(t)
1685+
logbuf:= strings.Builder{}
1686+
logger:=testutil.Logger(t).AppendSinks(sloghuman.Sink(&logbuf)).Leveled(slog.LevelDebug)
16831687

16841688
// GIVEN: an enqueuer & a sample user
16851689
cfg:=defaultNotificationsConfig(database.NotificationMethodSmtp)
@@ -1697,9 +1701,11 @@ func TestDisabledBeforeEnqueue(t *testing.T) {
16971701
require.NoError(t,err,"failed to set preferences")
16981702
require.EqualValues(t,1,n,"unexpected number of affected rows")
16991703

1700-
// THEN: enqueuing the "workspace deleted" notification should fail with an error
1704+
// THEN: enqueuing the "workspace deleted" notification should fail be
1705+
// a no-op that produces a debug log
17011706
_,err=enq.Enqueue(ctx,user.ID,templateID,map[string]string{},"test")
1702-
require.ErrorIs(t,err,notifications.ErrCannotEnqueueDisabledNotification,"enqueueing did not fail with expected error")
1707+
require.NoError(t,err)
1708+
require.Contains(t,logbuf.String(),"not enqueueing disabled notification")
17031709
}
17041710

17051711
// TestDisabledAfterEnqueue ensures that notifications enqueued before a notification template was disabled will not be
@@ -1909,7 +1915,8 @@ func TestNotificationDuplicates(t *testing.T) {
19091915

19101916
ctx:=dbauthz.AsNotifier(testutil.Context(t,testutil.WaitSuperLong))
19111917
store,pubsub:=dbtestutil.NewDB(t)
1912-
logger:=testutil.Logger(t)
1918+
logbuf:= strings.Builder{}
1919+
logger:=testutil.Logger(t).AppendSinks(sloghuman.Sink(&logbuf)).Leveled(slog.LevelDebug)
19131920

19141921
method:=database.NotificationMethodSmtp
19151922
cfg:=defaultNotificationsConfig(method)
@@ -1933,10 +1940,11 @@ func TestNotificationDuplicates(t *testing.T) {
19331940
map[string]string{"initiator":"danny"},"test",user.ID)
19341941
require.NoError(t,err)
19351942

1936-
// WHEN: the second is enqueued, the enqueuer will rejectthe request.
1943+
// WHEN: the second is enqueued, the enqueuer will rejectit as a duplicate.
19371944
_,err=enq.Enqueue(ctx,user.ID,notifications.TemplateWorkspaceDeleted,
19381945
map[string]string{"initiator":"danny"},"test",user.ID)
1939-
require.ErrorIs(t,err,notifications.ErrDuplicate)
1946+
require.NoError(t,err)
1947+
require.Contains(t,logbuf.String(),"not enqueueing duplicate notification")
19401948

19411949
// THEN: when the clock is advanced 24h, the notification will be accepted.
19421950
// NOTE: the time is used in the dedupe hash, so by advancing 24h we're creating a distinct notification from the one

‎coderd/notifications/reports/generator.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,7 @@ func reportFailedWorkspaceBuilds(ctx context.Context, logger slog.Logger, db dat
205205
reportData,
206206
"report_generator",
207207
slice.Unique(targets)...,
208-
);err!=nil&&notifications.IsSeriousEnqueueError(err){
208+
);err!=nil {
209209
logger.Warn(ctx,"failed to send a report with failed workspace builds",slog.Error(err))
210210
}
211211
}

‎coderd/provisionerdserver/provisionerdserver.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1313,7 +1313,7 @@ func (s *server) notifyWorkspaceBuildFailed(ctx context.Context, workspace datab
13131313
},"provisionerdserver",
13141314
// Associate this notification with all the related entities.
13151315
workspace.ID,workspace.OwnerID,workspace.TemplateID,workspace.OrganizationID,
1316-
);err!=nil&&notifications.IsSeriousEnqueueError(err){
1316+
);err!=nil {
13171317
s.Logger.Warn(ctx,"failed to notify of failed workspace autobuild",slog.Error(err))
13181318
}
13191319
}
@@ -1342,7 +1342,7 @@ func (s *server) notifyWorkspaceManualBuildFailed(ctx context.Context, workspace
13421342
labels,"provisionerdserver",
13431343
// Associate this notification with all the related entities.
13441344
workspace.ID,workspace.OwnerID,workspace.TemplateID,workspace.OrganizationID,
1345-
);err!=nil&&notifications.IsSeriousEnqueueError(err){
1345+
);err!=nil {
13461346
s.Logger.Warn(ctx,"failed to notify of failed workspace manual build",slog.Error(err))
13471347
}
13481348
}
@@ -2441,7 +2441,7 @@ func (s *server) notifyWorkspaceDeleted(ctx context.Context, workspace database.
24412441
},"provisionerdserver",
24422442
// Associate this notification with all the related entities.
24432443
workspace.ID,workspace.OwnerID,workspace.TemplateID,workspace.OrganizationID,
2444-
);err!=nil&&notifications.IsSeriousEnqueueError(err){
2444+
);err!=nil {
24452445
s.Logger.Warn(ctx,"failed to notify of workspace deletion",slog.Error(err))
24462446
}
24472447
}

‎coderd/templates.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ func (api *API) notifyTemplateDeleted(ctx context.Context, template database.Tem
151151
},"api-templates-delete",
152152
// Associate this notification with all the related entities.
153153
template.ID,template.OrganizationID,
154-
);err!=nil&&notifications.IsSeriousEnqueueError(err){
154+
);err!=nil {
155155
api.Logger.Warn(ctx,"failed to notify of template deletion",slog.F("deleted_template_id",template.ID),slog.Error(err))
156156
}
157157
}
@@ -968,7 +968,7 @@ func (api *API) notifyUsersOfTemplateDeprecation(ctx context.Context, template d
968968
},
969969
"notify-users-of-template-deprecation",
970970
)
971-
iferr!=nil&&notifications.IsSeriousEnqueueError(err){
971+
iferr!=nil {
972972
errs=append(errs,xerrors.Errorf("enqueue notification: %w",err))
973973
}
974974
}

‎coderd/userauth.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -317,7 +317,7 @@ func (api *API) notifyUserRequestedOneTimePasscode(ctx context.Context, user dat
317317
"change-password-with-one-time-passcode",
318318
user.ID,
319319
)
320-
iferr!=nil&&notifications.IsSeriousEnqueueError(err){
320+
iferr!=nil {
321321
returnxerrors.Errorf("enqueue notification: %w",err)
322322
}
323323

‎coderd/users.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -606,7 +606,7 @@ func (api *API) deleteUser(rw http.ResponseWriter, r *http.Request) {
606606
},
607607
"api-users-delete",
608608
user.ID,
609-
);err!=nil&&notifications.IsSeriousEnqueueError(err){
609+
);err!=nil {
610610
api.Logger.Warn(ctx,"unable to notify about deleted user",slog.F("deleted_user",user.Username),slog.Error(err))
611611
}
612612
}
@@ -951,15 +951,15 @@ func (api *API) notifyUserStatusChanged(ctx context.Context, actingUserName stri
951951
if_,err:=api.NotificationsEnqueuer.EnqueueWithData(dbauthz.AsNotifier(ctx),u.ID,adminTemplateID,
952952
labels,data,"api-put-user-status",
953953
targetUser.ID,
954-
);err!=nil&&notifications.IsSeriousEnqueueError(err){
954+
);err!=nil {
955955
api.Logger.Warn(ctx,"unable to notify about changed user's status",slog.F("affected_user",targetUser.Username),slog.Error(err))
956956
}
957957
}
958958
// nolint:gocritic // Need notifier actor to enqueue notifications
959959
if_,err:=api.NotificationsEnqueuer.EnqueueWithData(dbauthz.AsNotifier(ctx),targetUser.ID,personalTemplateID,
960960
labels,data,"api-put-user-status",
961961
targetUser.ID,
962-
);err!=nil&&notifications.IsSeriousEnqueueError(err){
962+
);err!=nil {
963963
api.Logger.Warn(ctx,"unable to notify user about status change of their account",slog.F("affected_user",targetUser.Username),slog.Error(err))
964964
}
965965
returnnil
@@ -1518,7 +1518,7 @@ func (api *API) CreateUser(ctx context.Context, store database.Store, req Create
15181518
},
15191519
"api-users-create",
15201520
user.ID,
1521-
);err!=nil&&notifications.IsSeriousEnqueueError(err){
1521+
);err!=nil {
15221522
api.Logger.Warn(ctx,"unable to notify about created user",slog.F("created_user",user.Username),slog.Error(err))
15231523
}
15241524
}

‎coderd/workspacebuilds.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -605,7 +605,7 @@ func (api *API) notifyWorkspaceUpdated(
605605
"api-workspaces-updated",
606606
// Associate this notification with all the related entities
607607
workspace.ID,workspace.OwnerID,workspace.TemplateID,workspace.OrganizationID,
608-
);err!=nil&&notifications.IsSeriousEnqueueError(err){
608+
);err!=nil {
609609
log.Warn(ctx,"failed to notify of workspace update",slog.Error(err))
610610
}
611611
}

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp