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

Commit0f29293

Browse files
committed
Review feedback
Signed-off-by: Danny Kopping <danny@coder.com>
1 parentba5f7c6 commit0f29293

File tree

18 files changed

+420
-167
lines changed

18 files changed

+420
-167
lines changed

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"github.com/open-policy-agent/opa/topdown"
1818

1919
"cdr.dev/slog"
20+
2021
"github.com/coder/coder/v2/coderd/rbac/policy"
2122
"github.com/coder/coder/v2/coderd/rbac/rolestore"
2223

@@ -1471,6 +1472,13 @@ func (q *querier) GetLogoURL(ctx context.Context) (string, error) {
14711472
returnq.db.GetLogoURL(ctx)
14721473
}
14731474

1475+
func (q*querier)GetNotificationMessagesByStatus(ctx context.Context,arg database.GetNotificationMessagesByStatusParams) ([]database.NotificationMessage,error) {
1476+
iferr:=q.authorizeContext(ctx,policy.ActionRead,rbac.ResourceSystem);err!=nil {
1477+
returnnil,err
1478+
}
1479+
returnq.db.GetNotificationMessagesByStatus(ctx,arg)
1480+
}
1481+
14741482
func (q*querier)GetOAuth2ProviderAppByID(ctx context.Context,id uuid.UUID) (database.OAuth2ProviderApp,error) {
14751483
iferr:=q.authorizeContext(ctx,policy.ActionRead,rbac.ResourceOauth2App);err!=nil {
14761484
return database.OAuth2ProviderApp{},err

‎coderd/database/dbmem/dbmem.go

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -944,10 +944,10 @@ func (q *FakeQuerier) AcquireNotificationMessages(_ context.Context, arg databas
944944
}
945945

946946
// Mimic mutation in database query.
947-
nm.UpdatedAt= sql.NullTime{Time:time.Now(),Valid:true}
947+
nm.UpdatedAt= sql.NullTime{Time:dbtime.Now(),Valid:true}
948948
nm.Status=database.NotificationMessageStatusLeased
949949
nm.StatusReason= sql.NullString{String:fmt.Sprintf("Enqueued by notifier %d",arg.NotifierID),Valid:true}
950-
nm.LeasedUntil= sql.NullTime{Time:time.Now().Add(time.Second*time.Duration(arg.LeaseSeconds)),Valid:true}
950+
nm.LeasedUntil= sql.NullTime{Time:dbtime.Now().Add(time.Second*time.Duration(arg.LeaseSeconds)),Valid:true}
951951

952952
out=append(out, database.AcquireNotificationMessagesRow{
953953
ID:nm.ID,
@@ -1836,7 +1836,7 @@ func (q *FakeQuerier) EnqueueNotificationMessage(_ context.Context, arg database
18361836
Targets:arg.Targets,
18371837
CreatedBy:arg.CreatedBy,
18381838
// Default fields.
1839-
CreatedAt:time.Now(),
1839+
CreatedAt:dbtime.Now(),
18401840
Status:database.NotificationMessageStatusPending,
18411841
}
18421842

@@ -2740,6 +2740,26 @@ func (q *FakeQuerier) GetLogoURL(_ context.Context) (string, error) {
27402740
returnq.logoURL,nil
27412741
}
27422742

2743+
func (q*FakeQuerier)GetNotificationMessagesByStatus(_ context.Context,arg database.GetNotificationMessagesByStatusParams) ([]database.NotificationMessage,error) {
2744+
err:=validateDatabaseType(arg)
2745+
iferr!=nil {
2746+
returnnil,err
2747+
}
2748+
2749+
varout []database.NotificationMessage
2750+
for_,m:=rangeq.notificationMessages {
2751+
iflen(out)>int(arg.Limit) {
2752+
returnout,nil
2753+
}
2754+
2755+
ifm.Status==arg.Status {
2756+
out=append(out,m)
2757+
}
2758+
}
2759+
2760+
returnout,nil
2761+
}
2762+
27432763
func (q*FakeQuerier)GetOAuth2ProviderAppByID(_ context.Context,id uuid.UUID) (database.OAuth2ProviderApp,error) {
27442764
q.mutex.Lock()
27452765
deferq.mutex.Unlock()

‎coderd/database/dbmetrics/dbmetrics.go

Lines changed: 7 additions & 0 deletions
Some generated files are not rendered by default. Learn more aboutcustomizing how changed files appear on GitHub.

‎coderd/database/dbmock/dbmock.go

Lines changed: 15 additions & 0 deletions
Some generated files are not rendered by default. Learn more aboutcustomizing how changed files appear on GitHub.

‎coderd/database/migrations/000221_notifications.up.sql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ CREATE INDEX idx_notification_messages_status ON notification_messages (status);
5252
-- TODO: autogenerate constants which reference the UUIDs
5353
INSERT INTO notification_templates (id, name, title_template, body_template,"group", actions)
5454
VALUES ('f517da0b-cdc9-410f-ab89-a86107c420ed','Workspace Deleted', E'Workspace "{{.Labels.name}}" deleted',
55-
E'Hi {{.UserName}}\n\nYour workspace **{{.Labels.name}}** was deleted.\nThe specified reason was "**{{.Labels.reason}}**".',
55+
E'Hi {{.UserName}}\n\nYour workspace **{{.Labels.name}}** was deleted.\nThe specified reason was "**{{.Labels.reason}}{{ if .Labels.initiatedBy }} ({{ .Labels.initiatedBy }}){{end}}**".',
5656
'Workspace Events','[
5757
{
5858
"label": "View workspaces",

‎coderd/database/querier.go

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more aboutcustomizing how changed files appear on GitHub.

‎coderd/database/queries.sql.go

Lines changed: 47 additions & 0 deletions
Some generated files are not rendered by default. Learn more aboutcustomizing how changed files appear on GitHub.

‎coderd/database/queries/notifications.sql

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,3 +125,6 @@ WHERE id IN
125125
FROM notification_messagesAS nested
126126
WHEREnested.updated_at< NOW()- INTERVAL'7 days');
127127

128+
-- name: GetNotificationMessagesByStatus :many
129+
SELECT*FROM notification_messagesWHERE status= @statusLIMITsqlc.arg('limit')::int;
130+

‎coderd/notifications/dispatch/smtp.go

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import (
2020

2121
"cdr.dev/slog"
2222

23-
"github.com/coder/coder/v2/coderd/database"
2423
"github.com/coder/coder/v2/coderd/notifications/render"
2524
"github.com/coder/coder/v2/coderd/notifications/types"
2625
markdown"github.com/coder/coder/v2/coderd/render"
@@ -53,10 +52,6 @@ func NewSMTPHandler(cfg codersdk.NotificationsEmailConfig, log slog.Logger) *SMT
5352
return&SMTPHandler{cfg:cfg,log:log}
5453
}
5554

56-
func (*SMTPHandler)NotificationMethod() database.NotificationMethod {
57-
returndatabase.NotificationMethodSmtp
58-
}
59-
6055
func (s*SMTPHandler)Dispatcher(payload types.MessagePayload,titleTmpl,bodyTmplstring) (DeliveryFunc,error) {
6156
// First render the subject & body into their own discrete strings.
6257
subject,err:=markdown.PlaintextFromMarkdown(titleTmpl)

‎coderd/notifications/dispatch/webhook.go

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import (
1313

1414
"cdr.dev/slog"
1515

16-
"github.com/coder/coder/v2/coderd/database"
1716
"github.com/coder/coder/v2/coderd/notifications/types"
1817
markdown"github.com/coder/coder/v2/coderd/render"
1918
"github.com/coder/coder/v2/codersdk"
@@ -40,11 +39,6 @@ func NewWebhookHandler(cfg codersdk.NotificationsWebhookConfig, log slog.Logger)
4039
return&WebhookHandler{cfg:cfg,log:log,cl:&http.Client{}}
4140
}
4241

43-
func (*WebhookHandler)NotificationMethod() database.NotificationMethod {
44-
// TODO: don't use database types
45-
returndatabase.NotificationMethodWebhook
46-
}
47-
4842
func (w*WebhookHandler)Dispatcher(payload types.MessagePayload,titleTmpl,bodyTmplstring) (DeliveryFunc,error) {
4943
ifw.cfg.Endpoint.String()=="" {
5044
returnnil,xerrors.New("webhook endpoint not defined")

‎coderd/notifications/manager.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@ import (
1616
"github.com/coder/coder/v2/codersdk"
1717
)
1818

19+
var (
20+
ErrInvalidDispatchTimeout=xerrors.New("dispatch timeout must be less than lease period")
21+
)
22+
1923
// Manager manages all notifications being enqueued and dispatched.
2024
//
2125
// Manager maintains a notifier: this consumes the queue of notification messages in the store.
@@ -53,6 +57,13 @@ type Manager struct {
5357
// helpers is a map of template helpers which are used to customize notification messages to use global settings like
5458
// access URL etc.
5559
funcNewManager(cfg codersdk.NotificationsConfig,storeStore,log slog.Logger) (*Manager,error) {
60+
// If dispatch timeout exceeds lease period, it is possible that messages can be delivered in duplicate because the
61+
// lease can expire before the notifier gives up on the dispatch, which results in the message becoming eligible for
62+
// being re-acquired.
63+
ifcfg.DispatchTimeout.Value()>=cfg.LeasePeriod.Value() {
64+
returnnil,ErrInvalidDispatchTimeout
65+
}
66+
5667
return&Manager{
5768
log:log,
5869
cfg:cfg,
@@ -82,6 +93,8 @@ func (m *Manager) WithHandlers(reg map[database.NotificationMethod]Handler) {
8293
// Manager requires system-level permissions to interact with the store.
8394
// Run is only intended to be run once.
8495
func (m*Manager)Run(ctx context.Context) {
96+
m.log.Info(ctx,"started")
97+
8598
m.runOnce.Do(func() {
8699
// Closes when Stop() is called or context is canceled.
87100
gofunc() {

‎coderd/notifications/manager_test.go

Lines changed: 16 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"encoding/json"
66
"sync/atomic"
77
"testing"
8-
"time"
98

109
"github.com/google/uuid"
1110
"github.com/stretchr/testify/assert"
@@ -15,8 +14,8 @@ import (
1514
"cdr.dev/slog"
1615
"cdr.dev/slog/sloggers/slogtest"
1716

18-
"github.com/coder/coder/v2/coderd/coderdtest"
1917
"github.com/coder/coder/v2/coderd/database"
18+
"github.com/coder/coder/v2/coderd/database/dbgen"
2019
"github.com/coder/coder/v2/coderd/database/dbmem"
2120
"github.com/coder/coder/v2/coderd/database/dbtestutil"
2221
"github.com/coder/coder/v2/coderd/notifications"
@@ -32,7 +31,7 @@ func TestBufferedUpdates(t *testing.T) {
3231
if!dbtestutil.WillUsePostgres() {
3332
t.Skip("This test requires postgres")
3433
}
35-
ctx,logger,db,ps:=setup(t)
34+
ctx,logger,db:=setup(t)
3635
interceptor:=&bulkUpdateInterceptor{Store:db}
3736

3837
santa:=&santaHandler{}
@@ -45,19 +44,15 @@ func TestBufferedUpdates(t *testing.T) {
4544
enq,err:=notifications.NewStoreEnqueuer(cfg,interceptor,defaultHelpers(),logger.Named("notifications-enqueuer"))
4645
require.NoError(t,err)
4746

48-
client:=coderdtest.New(t,&coderdtest.Options{Database:db,Pubsub:ps})
49-
user:=coderdtest.CreateFirstUser(t,client)
47+
user:=dbgen.User(t,db, database.User{})
5048

5149
// given
52-
if_,err:=enq.Enqueue(ctx,user.UserID,notifications.TemplateWorkspaceDeleted,map[string]string{"nice":"true"},"");true {
53-
require.NoError(t,err)
54-
}
55-
if_,err:=enq.Enqueue(ctx,user.UserID,notifications.TemplateWorkspaceDeleted,map[string]string{"nice":"true"},"");true {
56-
require.NoError(t,err)
57-
}
58-
if_,err:=enq.Enqueue(ctx,user.UserID,notifications.TemplateWorkspaceDeleted,map[string]string{"nice":"false"},"");true {
59-
require.NoError(t,err)
60-
}
50+
_,err=enq.Enqueue(ctx,user.ID,notifications.TemplateWorkspaceDeleted,map[string]string{"nice":"true"},"")
51+
require.NoError(t,err)
52+
_,err=enq.Enqueue(ctx,user.ID,notifications.TemplateWorkspaceDeleted,map[string]string{"nice":"true"},"")
53+
require.NoError(t,err)
54+
_,err=enq.Enqueue(ctx,user.ID,notifications.TemplateWorkspaceDeleted,map[string]string{"nice":"false"},"")
55+
require.NoError(t,err)
6156

6257
// when
6358
mgr.Run(ctx)
@@ -94,7 +89,6 @@ func TestBuildPayload(t *testing.T) {
9489
"my_url":func()string {returnurl },
9590
}
9691

97-
ctx:=context.Background()
9892
db:=dbmem.New()
9993
interceptor:=newEnqueueInterceptor(db,
10094
// Inject custom message metadata to influence the payload construction.
@@ -107,7 +101,7 @@ func TestBuildPayload(t *testing.T) {
107101
},
108102
}
109103
out,err:=json.Marshal(actions)
110-
require.NoError(t,err)
104+
assert.NoError(t,err)
111105

112106
return database.FetchNewMessageMetadataRow{
113107
NotificationName:"My Notification",
@@ -122,19 +116,17 @@ func TestBuildPayload(t *testing.T) {
122116
enq,err:=notifications.NewStoreEnqueuer(defaultNotificationsConfig(database.NotificationMethodSmtp),interceptor,helpers,logger.Named("notifications-enqueuer"))
123117
require.NoError(t,err)
124118

119+
ctx:=testutil.Context(t,testutil.WaitShort)
120+
125121
// when
126122
_,err=enq.Enqueue(ctx,uuid.New(),notifications.TemplateWorkspaceDeleted,nil,"test")
127123
require.NoError(t,err)
128124

129125
// then
130-
select {
131-
casepayload:=<-interceptor.payload:
132-
require.Len(t,payload.Actions,1)
133-
require.Equal(t,label,payload.Actions[0].Label)
134-
require.Equal(t,url,payload.Actions[0].URL)
135-
case<-time.After(testutil.WaitShort):
136-
t.Fatalf("timed out")
137-
}
126+
payload:=testutil.RequireRecvCtx(ctx,t,interceptor.payload)
127+
require.Len(t,payload.Actions,1)
128+
require.Equal(t,label,payload.Actions[0].Label)
129+
require.Equal(t,url,payload.Actions[0].URL)
138130
}
139131

140132
funcTestStopBeforeRun(t*testing.T) {
@@ -184,10 +176,6 @@ type santaHandler struct {
184176
nice atomic.Int32
185177
}
186178

187-
func (*santaHandler)NotificationMethod() database.NotificationMethod {
188-
returndatabase.NotificationMethodSmtp
189-
}
190-
191179
func (s*santaHandler)Dispatcher(payload types.MessagePayload,_,_string) (dispatch.DeliveryFunc,error) {
192180
returnfunc(ctx context.Context,msgID uuid.UUID) (retryablebool,errerror) {
193181
ifpayload.Labels["nice"]!="true" {

0 commit comments

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