NotificationsYou must be signed in to change notification settings
Fork928
Star10.1k

feat: send native system notification on scheduled workspace shutdown#1414

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

Jump to bottom

Merged

johnstcn merged 6 commits intomainfromcj/gh-861/cli-autostop-notify

May 13, 2022

Merged

feat: send native system notification on scheduled workspace shutdown#1414

johnstcn merged 6 commits intomainfromcj/gh-861/cli-autostop-notify

May 13, 2022

Conversation

Copy link

Member

johnstcn commentedMay 12, 2022•
edited
Loading

This commit adds a fairly generic notification package and uses it
to notify users connected over SSH of pending workspace shutdowns.

It usesgithub.com/gen2brain/beeep under the hood for actually doing the platform-specific heavy lifting.

I've tested it out on MacOS and Windows and it seems to work fine there; I don't have a Linux desktop around with dbus and all of that associated stuff setup to test it there though.

Theautobuild/notify package might be a bit of a step too far, but it lets me easily test the notification timing logic without worrying about anything else -- which is arguably the most important part.

Subtasks

Added a basic proof-of-concept
Refactored notification scheduling logic to its own package, unit tests.
Moved the actual notification meat and bones to the SSH command.

Additions

Ensured only one process at a time can notify per workspace (usinggofrs/flock)
Added a convenience methodnotifier.Notify for folks who don't like making their own timers.

Fixes#861

feat: send native system notification on scheduled workspace shutdown

125584c

This commit adds a fairly generic notification package and uses itto notify users connected over SSH of pending workspace shutdowns.

johnstcn requested a review froma team

May 12, 2022 21:08

johnstcn self-assigned this

May 12, 2022

f0ssel reviewed

May 12, 2022

View reviewed changes

cli/ssh.go OutdatedShow resolvedHide resolved

Copy link

codecovbot commentedMay 12, 2022•
edited
Loading

Codecov Report

Merging#1414 (4f39908) intomain (64e408c) willincrease coverage by0.08%.
The diff coverage is70.96%.

@@            Coverage Diff             @@##             main    #1414      +/-   ##==========================================+ Coverage   67.13%   67.22%   +0.08%==========================================  Files         287      288       +1       Lines       19307    19424     +117       Branches      241      244       +3     ==========================================+ Hits        12962    13057      +95- Misses       5007     5025      +18- Partials     1338     1342       +4

Flag	Coverage Δ
unittest-go-macos-latest	`54.28% <70.96%> (+0.13%)`	⬆️
unittest-go-postgres-	`65.56% <70.96%> (-0.03%)`	⬇️
unittest-go-ubuntu-latest	`56.65% <70.96%> (+0.17%)`	⬆️
unittest-go-windows-2022	`52.65% <70.96%> (+0.13%)`	⬆️
unittest-js	`75.26% <ø> (+0.61%)`	⬆️

Impacted Files	Coverage Δ
cli/ssh.go	`39.53% <35.71%> (-1.24%)`	⬇️
coderd/autobuild/notify/notifier.go	`100.00% <100.00%> (ø)`
site/src/components/Section/Section.tsx	`61.11% <0.00%> (-5.56%)`	⬇️
site/src/xServices/auth/authXService.ts	`78.12% <0.00%> (-4.49%)`	⬇️
coderd/workspaceagents.go	`55.34% <0.00%> (-2.56%)`	⬇️
peer/channel.go	`85.96% <0.00%> (ø)`
coderd/rbac/object.go	`100.00% <0.00%> (ø)`
coderd/provisionerdaemons.go	`63.31% <0.00%> (ø)`
provisionerd/provisionerd.go	`77.37% <0.00%> (ø)`
site/src/xServices/auth/authSelectors.ts	`100.00% <0.00%> (ø)`
... and8 more

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update64e408c...4f39908. Read thecomment docs.

Copy link

Member

bpmct commentedMay 12, 2022•
edited
Loading

I've tested it out on MacOS and Windows and it seems to work fine there; I don't have a Linux desktop around with dbus and all of that associated stuff setup to test it there though.

Sounds like we need to add some templates for test desktops on v2 dogfood

ammario requested changes

May 13, 2022

View reviewed changes

Copy link

Member

ammario left a comment

Choose a reason for hiding this comment

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

Per my other comment, if the notification triggers onevery open coder-cli instance, then this may overwhelm the user.

notifier: refactor, add more unit tests

8313a52

mafredri reviewed

May 13, 2022

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.

Sorry for the sheer amount of comments 😅. I like how you managed to keep the condition logic simple!

coderd/autobuild/notify/notifier.go OutdatedShow resolvedHide resolved

cli/ssh.go OutdatedShow resolvedHide resolved

coderd/autobuild/notify/notifier.goShow resolvedHide resolved

coderd/autobuild/notify/notifier_test.go OutdatedShow resolvedHide resolved

coderd/autobuild/notify/notifier_test.go

		t.Run(testCase.Name, func(t *testing.T) {
		ch := make(chan time.Time)
		numConditions := atomic.NewInt64(0)
		numCalls := atomic.NewInt64(0)

Copy link

Member

mafredriMay 13, 2022

Choose a reason for hiding this comment

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

Cool use ofatomic, I need to remember this!☺️

coderd/autobuild/notify/notifier_test.goShow resolvedHide resolved

johnstcn added2 commits

May 13, 2022 10:41

address PR comments

67516c7

fixup! address PR comments

55a1de2

mafredri reviewed

May 13, 2022

View reviewed changes

cli/ssh.go OutdatedShow resolvedHide resolved

use gofrs/flock instead to handle file locking

e830baf

johnstcn requested review frommafredri,ammario andf0ssel

May 13, 2022 11:37

mafredri approved these changes

May 13, 2022

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.

Just one more question but overall very nice!

coderd/autobuild/notify/notifier.go

		type Notifier struct {
		lock sync.Mutex
		condition Condition
		notifiedAt map[time.Duration]bool

Copy link

Member

mafredriMay 13, 2022

Choose a reason for hiding this comment

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

Would it make sense to move this intopollOnce? My thought is that this map never reset, and a notification can only be shown once percountdown entry (for the lifetime of theNotifier). Not sure if that's a problem or not though.

Copy link

MemberAuthor

johnstcnMay 13, 2022

Choose a reason for hiding this comment

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

We could move intoPoll, but not intopollOnce.
If we did that, we could probably also remove thesync.Mutex.
I like the idea of the sameNotifier instance keeping track of what it's already notified between successive calls toPoll() though.

Copy link

Member

mafredriMay 13, 2022

Choose a reason for hiding this comment

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

Ok that makes sense. I guess it's mostly a question of how the package is to be used and what the user (might) expect to happen. I imagine this will not be a problem forssh since when the workspace stops, so does the command, so the notifier doesn't outlive its purpose.

kylecarbs reviewed

May 13, 2022

View reviewed changes

cli/ssh.go

Comment on lines +196 to +212

		// Attempt to poll workspace autostop. We write a per-workspace lockfile to
		// avoid spamming the user with notifications in case of multiple instances
		// of the CLI running simultaneously.
		func tryPollWorkspaceAutostop(ctx context.Context, client *codersdk.Client, workspace codersdk.Workspace) (stop func()) {
		lock := flock.New(filepath.Join(os.TempDir(), "coder-autostop-notify-"+workspace.ID.String()))
		condition := notifyCondition(ctx, client, workspace.ID, lock)
		return notify.Notify(condition, autostopPollInterval, autostopNotifyCountdown...)
		}

		// Notify the user if the workspace is due to shutdown.
		func notifyCondition(ctx context.Context, client codersdk.Client, workspaceID uuid.UUID, lock flock.Flock) notify.Condition {
		return func(now time.Time) (deadline time.Time, callback func()) {
		// Keep trying to regain the lock.
		locked, err := lock.TryLockContext(ctx, autostopPollInterval)
		if err != nil \|\| !locked {
		return time.Time{}, nil
		}

Copy link

Member

kylecarbsMay 13, 2022

Choose a reason for hiding this comment

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

I wonder if it'd be best to place as much of this as possible in thenotify package. eg. forcoder port-forward we'd probably want to do the exact same type of thing.

I could imagine for the frontend too, we would send a notification via WebSocket or something!

Copy link

MemberAuthor

johnstcnMay 13, 2022

Choose a reason for hiding this comment

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

That could be a good idea when we have a better idea of what different use-cases crop up.
I fear I've already over-engineered this enough as it is!

ammario approved these changes

May 13, 2022

View reviewed changes

Copy link

Member

ammario left a comment

Choose a reason for hiding this comment

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

I didn't review the code, but if onlyone notification is sent acrossall coder-clis, I'm good with this.

notify only once at most five minutes before shutdown

4f39908

johnstcn merged commitb2760b1 intomain

May 13, 2022

johnstcn deleted the cj/gh-861/cli-autostop-notify branch

May 13, 2022 17:09

misskniss added the V2 BETA label

May 15, 2022

misskniss added this to theV2 Beta milestone

May 15, 2022

greyscaled mentioned this pull request

May 21, 2022

CLI auto-stop extension#1461

Closed

kylecarbs pushed a commit that referenced this pull request

Jun 10, 2022

feat: send native system notification on scheduled workspace shutdown (…

4051baf

…#1414)* feat: send native system notification on scheduled workspace shutdownThis commit adds a fairly generic notification package and uses itto notify users connected over SSH of pending workspace shutdowns.Only one notification will be sent at most 5 minutes prior to the scheduledshutdown, and only one CLI instance will send notifications if multipleinstances are running.

Labels

None yet

7 participants

Movatterモバイル変換

feat: send native system notification on scheduled workspace shutdown#1414

feat: send native system notification on scheduled workspace shutdown#1414

Uh oh!

Conversation

johnstcn commentedMay 12, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Subtasks

Additions

Uh oh!

Uh oh!

codecovbot commentedMay 12, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bpmct commentedMay 12, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

ammario left a comment

Choose a reason for hiding this comment

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mafredriMay 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

mafredriMay 13, 2022

Choose a reason for hiding this comment

Uh oh!

johnstcnMay 13, 2022

Choose a reason for hiding this comment

Uh oh!

mafredriMay 13, 2022

Choose a reason for hiding this comment

Uh oh!

kylecarbsMay 13, 2022

Choose a reason for hiding this comment

Uh oh!

johnstcnMay 13, 2022

Choose a reason for hiding this comment

Uh oh!

ammario left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

johnstcn commentedMay 12, 2022•
edited
Loading

codecovbot commentedMay 12, 2022•
edited
Loading

bpmct commentedMay 12, 2022•
edited
Loading