~~Adds a new option ($CODER_NOTIFICATIONS_ENABLED) that allows disabling notifications. This defaults totrue to avoid disabling notifications for existing installations of Coder.~~

Disables notifications when both$CODER_NOTIFICATIONS_WEBHOOK_ENDPOINT and$CODER_EMAIL_SMARTHOST are unset.

Drops the default value for$CODER_EMAIL_SMARTHOST.

DanielleMaywoodand others added11 commits

November 12, 2024 13:44

fix: remove defaults for CODER_EMAIL_ options

5c36c93

fix: failing tests

196f538

fix: tests (again)

b670009

fix: run 'make gen'

19398cc

fix: consistency

4175bc8

Co-authored-by: Danny Kopping <danny@coder.com>

nit: trim smarthost

66d1a69

nit: re-add default for CODER_EMAIL_HELLO

4689e5b

temporarily using new branch from coder/serpent for this

fix: run 'make {update-golden-files,gen}'

01d4957

fix: run 'make fmt'

27fa5bf

feat: allow disabling notifications entirely

6ca7844

Merge branch 'main' into dm-fix-defaults-notifications

fb54e45

github-actionsbot assignedDanielleMaywood

Nov 14, 2024

DanielleMaywood added3 commits

November 14, 2024 10:16

chore: re-add default for smarthost

d5aff28

fix: remove default for deprecated option

01cb999

chore: revert change

c513113

DanielleMaywood marked this pull request as ready for review

November 14, 2024 13:13

DanielleMaywood requested review fromdannykopping,johnstcn andmafredri

November 14, 2024 13:25

dannykopping reviewed

Nov 14, 2024

View reviewed changes

codersdk/deployment_test.go OutdatedShow resolvedHide resolved

codersdk/deployment.go OutdatedShow resolvedHide resolved

chore: remove unrelated test changes

a8b5a61

dannykopping reviewed

Nov 14, 2024

View reviewed changes

codersdk/deployment.go OutdatedShow resolvedHide resolved

revert: more changes

1711e68

mafredri approved these changes

Nov 14, 2024

View reviewed changes

Copy link

Member

mafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Minor nits but otherwise LGTM 👍🏻

cli/server.go OutdatedShow resolvedHide resolved

codersdk/deployment.go OutdatedShow resolvedHide resolved

DanielleMaywoodand others added4 commits

November 14, 2024 15:28

chore: simplify description

2e03ed1

Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>

Merge branch 'main' into dm-fix-defaults-notifications

28ec1b7

chore: make {gen,update-golden-files}

7ba3a6c

chore: rename cfg to notificationsCfg

4a72e4f

johnstcn approved these changes

Nov 15, 2024

View reviewed changes

Copy link

Member

johnstcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Nothing blocking from my side!

chore: drop enabled flag

9c02842

fix: invalid test

dd527f9

Copy link

ContributorAuthor

DanielleMaywood commentedNov 19, 2024

I've made some changes to the approach taken here due to feedback in#15513. Rather than having a flag that enables/disables notifications, we figure it out based on if certain env variables are set. To do this we are making a breaking change by dropping the default forCODER_EMAIL_SMARTHOST, although this shouldn't be a major issue as anyone using SMTP notifications will most likely have changed this setting anyway.

dannykopping reviewed

Nov 19, 2024

View reviewed changes

Copy link

Contributor

dannykopping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looking good, a couple more things to tweak and we can get this merged today 👍

cli/server.go OutdatedShow resolvedHide resolved

codersdk/deployment.goShow resolvedHide resolved

dannykopping reviewed

Nov 19, 2024

View reviewed changes

codersdk/deployment.goShow resolvedHide resolved

DanielleMaywood added2 commits

November 19, 2024 10:25

chore: improve info message and add to docs

77d1803

test: notifications.Enabled()

5fd03b3

dannykopping approved these changes

Nov 19, 2024

View reviewed changes

Copy link

Contributor

dannykopping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM, thanks@DanielleMaywood!

codersdk/deployment_test.goShow resolvedHide resolved

Copy link

Member

johnstcn commentedNov 19, 2024

I've made some changes to the approach taken here due to feedback in#15513. Rather than having a flag that enables/disables notifications, we figure it out based on if certain env variables are set. To do this we are making a breaking change by dropping the default forCODER_EMAIL_SMARTHOST, although this shouldn't be a major issue as anyone using SMTP notifications will most likely have changed this setting anyway.

Gotcha! FYI, if we do a breaking change, we should indicate that in the commit message (e.g.feat!: ...