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

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

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

Merged
BrunoQuaresma merged 44 commits intomainfrombq/implement-notifications
Jul 24, 2024

Conversation

BrunoQuaresma
Copy link
Collaborator

@BrunoQuaresmaBrunoQuaresma commentedJul 10, 2024•
edited
Loading

MrPeacockNLB reacted with thumbs up emoji
@BrunoQuaresmaBrunoQuaresma marked this pull request as ready for reviewJuly 10, 2024 18:50
Copy link
Contributor

@dannykoppingdannykopping left a comment

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

@BrunoQuaresma
Copy link
CollaboratorAuthor

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

@dannykopping
Copy link
Contributor

@BrunoQuaresma I tried to move this along while you were out, but I didn't have much time.
I added467a797 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.

BrunoQuaresma reacted with thumbs up emoji

Co-authored-by: Danny Kopping <danny@coder.com>
@dannykoppingdannykopping marked this pull request as draftJuly 18, 2024 16:00
@BrunoQuaresmaBrunoQuaresmaforce-pushed thebq/implement-notifications branch fromba7fbd9 to299cd7fCompareJuly 18, 2024 16:03
@mtojek
Copy link
Member

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

@dannykopping
Copy link
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.

mtojek reacted with thumbs up emoji

@BrunoQuaresmaBrunoQuaresma marked this pull request as ready for reviewJuly 23, 2024 16:44
@BrunoQuaresma
Copy link
CollaboratorAuthor

@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

Copy link
Member

@mtojekmtojek left a comment

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
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 markeddormant because of{reason}.

Append if dormancy cleanup is enabled anddormancy_hours < 24:

Dormant workspaces areautomatically deleted after{dormancy_hours} hours of inactivity.

Append if dormancy cleanup is enabled, anddormancy_hours > 24:

Dormant workspaces areautomatically 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"

BrunoQuaresma reacted with thumbs up emoji
Copy link
CollaboratorAuthor

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.

@BrunoQuaresmaBrunoQuaresma merged commit0d9615b intomainJul 24, 2024
@BrunoQuaresmaBrunoQuaresma deleted the bq/implement-notifications branchJuly 24, 2024 16:38
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJul 24, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@dannykoppingdannykoppingdannykopping approved these changes

@mtojekmtojekmtojek left review comments

@stirbystirbystirby left review comments

Assignees

@BrunoQuaresmaBrunoQuaresma

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@BrunoQuaresma@dannykopping@mtojek@stirby@sreya
[8]ページ先頭
©2009-2025 Movatter.jp