feat: allow removing deadline for running workspace #16085
Conversation
141c890 to
5ed74c0
Compare
| func TestExecutorWorkspaceAutostopNoWaitChangedMyMind(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| var ( | ||
| ctx = context.Background() | ||
| tickCh = make(chan time.Time) | ||
| statsCh = make(chan autobuild.Stats) | ||
| client = coderdtest.New(t, &coderdtest.Options{ | ||
| AutobuildTicker: tickCh, | ||
| IncludeProvisionerDaemon: true, | ||
| AutobuildStats: statsCh, | ||
| }) | ||
| // Given: we have a user with a workspace | ||
| workspace = mustProvisionWorkspace(t, client) | ||
| ) | ||
|
|
||
| // Given: the user changes their mind and decides their workspace should not autostop | ||
| err := client.UpdateWorkspaceTTL(ctx, workspace.ID, codersdk.UpdateWorkspaceTTLRequest{TTLMillis: nil}) | ||
| require.NoError(t, err) | ||
|
|
||
| // Then: the deadline should still be the original value | ||
| updated := coderdtest.MustWorkspace(t, client, workspace.ID) | ||
| assert.WithinDuration(t, workspace.LatestBuild.Deadline.Time, updated.LatestBuild.Deadline.Time, time.Minute) | ||
|
|
||
| // When: the autobuild executor ticks after the original deadline | ||
| go func() { | ||
| tickCh <- workspace.LatestBuild.Deadline.Time.Add(time.Minute) | ||
| }() | ||
|
|
||
| // Then: the workspace should stop | ||
| stats := <-statsCh | ||
| assert.Len(t, stats.Errors, 0) | ||
| assert.Len(t, stats.Transitions, 1) | ||
| assert.Equal(t, stats.Transitions[workspace.ID], database.WorkspaceTransitionStop) | ||
|
|
||
| // Wait for stop to complete | ||
| updated = coderdtest.MustWorkspace(t, client, workspace.ID) | ||
| _ = coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, updated.LatestBuild.ID) | ||
|
|
||
| // Start the workspace again | ||
| workspace = coderdtest.MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStop, database.WorkspaceTransitionStart) | ||
|
|
||
| // Given: the user changes their mind again and wants to enable autostop | ||
| newTTL := 8 * time.Hour | ||
| err = client.UpdateWorkspaceTTL(ctx, workspace.ID, codersdk.UpdateWorkspaceTTLRequest{TTLMillis: ptr.Ref(newTTL.Milliseconds())}) | ||
| require.NoError(t, err) | ||
|
|
||
| // Then: the deadline should remain at the zero value | ||
| updated = coderdtest.MustWorkspace(t, client, workspace.ID) | ||
| assert.Zero(t, updated.LatestBuild.Deadline) | ||
|
|
||
| // When: the relentless onward march of time continues | ||
| go func() { | ||
| tickCh <- workspace.LatestBuild.Deadline.Time.Add(newTTL + time.Minute) | ||
| close(tickCh) | ||
| }() | ||
|
|
||
| // Then: the workspace should not stop | ||
| stats = <-statsCh | ||
| assert.Len(t, stats.Errors, 0) | ||
| assert.Len(t, stats.Transitions, 0) | ||
| } | ||
|
|
There was a problem hiding this comment.
While it would technically be duplication, I feel like it would be a good idea to rework this test into the desired shape. Rationale: it's related to autostop behaviour, so the first place folks will go to look is in this package. I don't think it has to be the exact same behaviour though; it can simply be the following flow:
Given: an existing running workspace with a TTL
When: the user disables TTL on the workspace
Then: the running workspace build has the TTL removed and executor does nothing to it.
There was a problem hiding this comment.
Sure thing, I'll rework this test then
| { | ||
| name: "IncreaseAutostopDoesNotModifyDeadline", | ||
| fromTTL: ptr.Ref((4 * time.Hour).Milliseconds()), | ||
| toTTL: ptr.Ref((8 * time.Hour).Milliseconds()), | ||
| afterUpdate: func(t *testing.T, before, after codersdk.NullTime) { | ||
| require.NotZero(t, before) | ||
| require.NotZero(t, after) | ||
| require.Equal(t, before, after) | ||
| }, | ||
| }, | ||
| { | ||
| name: "DecreaseAutostopDoesNotModifyDeadline", | ||
| fromTTL: ptr.Ref((8 * time.Hour).Milliseconds()), | ||
| toTTL: ptr.Ref((4 * time.Hour).Milliseconds()), | ||
| afterUpdate: func(t *testing.T, before, after codersdk.NullTime) { | ||
| require.NotZero(t, before) | ||
| require.NotZero(t, after) | ||
| require.Equal(t, before, after) | ||
| }, | ||
| }, |
There was a problem hiding this comment.
praise, non-blocking: I feel like this requirement may change in future. These tests look very simple to modify in that case, so 👍 from me here
| if ( | ||
| data.autostopChanged && | ||
| getAutostop(workspace).autostopEnabled | ||
| ) { |
There was a problem hiding this comment.
My gut tells me that this will need an update to WorkspaceSchedulePage.stories.tsx.
There was a problem hiding this comment.
I'll take a look 👍
There was a problem hiding this comment.
Looks like there aren't any existing stories covering hitting the "Save" button on the form.
| }, | ||
| }, | ||
| { | ||
| name: "AddAutostopDoesNotAddDeadline", |
There was a problem hiding this comment.
Perhaps I'm thinking of this wrong, but it seems a bit weird that removing autostop removes a deadline, but adding it doesn't add one. Then again, deciding whether or not that deadline should be applied from workspace start, last activity or now is perhaps why it's not that way, oh well.. 😄
Fixes #9775
When a workspace's TTL is removed, and the workspace is running, the deadline is removed from the workspace.
This also modifies the frontend to not show a confirmation dialog when the change is to remove autostop.