- Notifications
You must be signed in to change notification settings - Fork927
chore: remove notifications experiment#14869
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
3148702 to6add98fCompare@dannykopping thanks for calling out that note. We should ship as is and revisit this before the next release. I'd opt to keep the defaults with a note in the docs rather than introduce friction in backward compatibility. To improve visibility, I can link to this PR in the release notes for 2.16.0. We get a lot of negative feedback on the difficulty of reverting upgrades, and it'd be unfortunate for a downgrade to override their settings. Can we merge this ASAP so I can pull it into the release branch? Just whenever you all are online. (cc@mtojek) |
| @@ -49,7 +49,7 @@ func NewReportGenerator(ctx context.Context, logger slog.Logger, db database.Sto | |||
| return nil | |||
| } | |||
| err = reportFailedWorkspaceBuilds(ctx, logger,db, enqueuer, clk) | |||
| err = reportFailedWorkspaceBuilds(ctx, logger,tx, enqueuer, clk) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Passing the maindb object through caused a deadlock; we need to pass the transaction-scoped db object instead.
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
…rovisionerDaemonSigned-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
593c5a0 to0107ad1CompareI've added the beta badge:
|
Signed-off-by: Danny Kopping <danny@coder.com>
5bffd26 to0aa6d7bCompare| @@ -148,11 +148,12 @@ const DeploymentSettingsNavigation: FC<DeploymentSettingsNavigationProps> = ({ | |||
| Users | |||
| </SidebarNavSubItem> | |||
| )} | |||
| {experiments.includes("notifications") && ( | |||
| <Stack direction={"row"} alignItems={"center"} css={{ gap: 0 }}> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit: When passing string values directly you can pass the value directly without the brackets:
direction="row"I tried to QA this locally, but I don't see the option in the sidebar when starting the server without any experiments set. Is it expected? I'm not sure how we are handling beta features tho. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
UI and code look good! 👍
Signed-off-by: Danny Kopping <danny@coder.com>
11f7b1b intomainUh oh!
There was an error while loading.Please reload this page.
Notifications have proved stable in the [mainline release ofv2.15](https://github.com/coder/coder/releases/tag/v2.15.0), and inpreparation for v2.16 we're moving this to stable.(cherry picked from commit11f7b1b)
Uh oh!
There was an error while loading.Please reload this page.
Notifications have proved stable in themainline release of v2.15, and in preparation for v2.16 we're moving this to stable.
Note:
Assuming a config which relies on the defaults for notifications settings, customers will now start seeing logs like this (assuming they don't have an open SMTP server on localhost):
This occurs because the default notification method is set as
smtpinCODER_NOTIFICATIONS_METHOD, and other defaults are configured (CODER_NOTIFICATIONS_EMAIL_SMARTHOSTandCODER_NOTIFICATIONS_EMAIL_HELLO).This is not harmful, but it isavoidable. We'd have to remove the default values for
CODER_NOTIFICATIONS_EMAIL_SMARTHOSTandCODER_NOTIFICATIONS_EMAIL_HELLO, and detect their absence, and log a warning with a link to the docs.Effectively it amounts to the same thing, but it's a little more user-friendly.
The challenge with this approach, though, is it might remove some actually useful default values for customers. We could cover this in release notes, but it weakens our backwards-compatibility posture. It's not strictly a break, but it could cause unintended behaviour if operators blindly upgrade without reading the notes (everybody reads the release notes, right? RIGHT?)and they rely on these defaults.
I think a good middleground could be to mention the docs in the above error log. Then, when we implement#14644, we remove the default SMTP values and set a
nonemethod by default.In retrospect I should've tried to find a way to not define any defaults here, but we live and learn.