- Notifications
You must be signed in to change notification settings - Fork917
feat: allow removing deadline for running workspace#16085
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
feat: allow removing deadline for running workspace#16085
Uh oh!
There was an error while loading.Please reload this page.
Conversation
141c890 to5ed74c0Compare| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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'srelated 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My gut tells me that this will need an update toWorkspaceSchedulePage.stories.tsx.
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.
I'll take a look 👍
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.
Looks like there aren't any existing stories covering hitting the "Save" button on the form.
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.
Cian raised some good points, no blockers on my part so approving 👍🏻
| }, | ||
| }, | ||
| { | ||
| name: "AddAutostopDoesNotAddDeadline", |
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.
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.. 😄
7c595e2 intomainUh oh!
There was an error while loading.Please reload this page.
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.