Skip to content

feat(coderd): notify when workspace is marked as dormant - #13868

Merged
BrunoQuaresma merged 44 commits into
mainfrom
bq/implement-notifications
Jul 24, 2024
Merged

feat(coderd): notify when workspace is marked as dormant#13868
BrunoQuaresma merged 44 commits into
mainfrom
bq/implement-notifications

Conversation

@BrunoQuaresma

@BrunoQuaresma BrunoQuaresma commented Jul 10, 2024

Copy link
Copy Markdown
Contributor

Related to coder/internal#7

@BrunoQuaresma
BrunoQuaresma marked this pull request as ready for review July 10, 2024 18:50

@dannykopping dannykopping left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good work, left a few thoughts

Comment thread coderd/dormancy/notifications.go Outdated
Comment thread coderd/workspaces.go Outdated
Comment thread coderd/workspaces.go Outdated
Comment thread coderd/dormancy/notifications.go
Comment thread coderd/workspaces_test.go Outdated
Comment thread coderd/workspaces_test.go Outdated
Comment thread coderd/workspaces.go Outdated
Comment thread coderd/database/migrations/000221_notifications.up.sql Outdated
Comment thread coderd/workspaces_test.go Outdated
Comment thread coderd/workspaces_test.go
@BrunoQuaresma

Copy link
Copy Markdown
Contributor Author

@dannykopping, since the last changes are quite significant, I would recommend reviewing using the commits instead of the full diff.

Comment thread coderd/database/migrations/000223_dormancy_notification_template.up.sql Outdated
Comment thread coderd/workspaces.go Outdated
Comment thread coderd/autobuild/lifecycle_executor.go Outdated
Comment thread enterprise/coderd/schedule/template.go Outdated
Comment thread enterprise/coderd/schedule/template_test.go Outdated
Comment thread coderd/database/dbauthz/dbauthz.go
Comment thread coderd/database/dbauthz/dbauthz.go
Comment thread enterprise/coderd/dormancy/notifications.go Outdated
Comment thread coderd/workspaces.go Outdated
Comment thread enterprise/coderd/schedule/template.go Outdated
Comment thread coderd/database/migrations/000226_dormancy_notification_template.up.sql Outdated
Comment thread coderd/schedule/mock.go Outdated
@dannykopping

Copy link
Copy Markdown
Contributor

@BrunoQuaresma I tried to move this along while you were out, but I didn't have much time.
I added 467a797 which simplifies the implementation, although I have not had time to test it thoroughly; feel free to revert if it causes issues.

I think we should pass the enqueuer during the instantiation phase of the various areas, rather than passing the enqueuer/logger at the last moment of responsibility like you were doing before.

I fixed the migration and added some detail to the notification.

Comment thread coderd/autobuild/lifecycle_executor.go
Comment thread coderd/schedule/mock.go Outdated
Comment thread coderd/workspaces.go Outdated
Co-authored-by: Danny Kopping <danny@coder.com>
@dannykopping
dannykopping marked this pull request as draft July 18, 2024 16:00
@BrunoQuaresma
BrunoQuaresma force-pushed the bq/implement-notifications branch from ba7fbd9 to 299cd7f Compare July 18, 2024 16:03
@mtojek

mtojek commented Jul 19, 2024

Copy link
Copy Markdown
Member

@BrunoQuaresma is it still a draft PR or a regular one?

@dannykopping

Copy link
Copy Markdown
Contributor

@BrunoQuaresma is it still a draft PR or a regular one?

I made it draft so that we don't merge accidentally before testing it fully, since it has approvals already.

@BrunoQuaresma
BrunoQuaresma marked this pull request as ready for review July 23, 2024 16:44
@BrunoQuaresma

Copy link
Copy Markdown
Contributor Author

@dannykopping @mtojek The biggest changes since the last review:

  • Added a test for the dormant notification in the lifecycle executor
  • Created a different template for "Marked for Deletion" since it is a bit different from the "Marked as Dormant" use case
  • Added tests to verify if notifications are sent when the dormant TTL is changed and if affected workspaces are getting notified

Comment thread coderd/autobuild/lifecycle_executor.go Outdated
Comment thread coderd/autobuild/lifecycle_executor_test.go Outdated
Comment thread coderd/autobuild/lifecycle_executor_test.go
Comment thread coderd/database/migrations/000229_dormancy_notification_template.up.sql Outdated
Comment thread enterprise/coderd/schedule/template.go Outdated
Comment thread enterprise/coderd/schedule/template_test.go
Comment thread enterprise/coderd/schedule/template_test.go Outdated
Comment thread enterprise/coderd/schedule/template_test.go

@mtojek mtojek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I seconded a few Danny's comments.

@BrunoQuaresma If you feel a need to clarify some comments, please sync with @dannykopping. Let's not elongate this task anymore, it is already a leftover from the last sprint.

'0ea69165-ec14-4314-91f1-69566ac3c5a0',
'Workspace Marked as Dormant',
E'Workspace "{{.Labels.name}}" marked as dormant',
E'Hi {{.UserName}}\n\n' || E'Your workspace **{{.Labels.name}}** has been marked as **dormant**.\n' || E'The specified reason was "**{{.Labels.reason}} (initiated by: {{ .Labels.initiator }}){{end}}**\n\n' || E'Dormancy refers to a workspace being unused for a defined length of time, and after it exceeds {{.Labels.dormancyHours}} hours of dormancy might be deleted.\n' || E'To activate your workspace again, simply use it as normal.',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The wording is a little weird here. Can we change it and add a link to the dormancy docs:

I don't think we need to include the initiator, just improve the reasons (see end of message).

Hi {user-name}, your workspace {ws-name} has been marked dormant because of {reason}.

Append if dormancy cleanup is enabled and dormancy_hours < 24:

Dormant workspaces are automatically deleted after {dormancy_hours} hours of inactivity.

Append if dormancy cleanup is enabled, and dormancy_hours > 24:

Dormant workspaces are automatically deleted after {dormancy_hours // 24} days of inactivity.

To prevent deletion, use your workspace with the link below.


The reasons also fit into differing grammatic context:

  • Lifecycle executor: "breached the template's threshold for inactivity"
  • API: "requested by user"
  • Template: "template updated to new dormancy policy"

If we use these, they'd fit the above messages better.

  • Lifcycle: "prolonged inactivity, exceeding the dormancy threshold"
  • API: "a user request"
  • Template: "an update to the template's dormancy"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Going to refactor the templates in the next PR to avoid further postponement.

@BrunoQuaresma
BrunoQuaresma merged commit 0d9615b into main Jul 24, 2024
@BrunoQuaresma
BrunoQuaresma deleted the bq/implement-notifications branch July 24, 2024 16:38
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants