Skip to content

feat: offer to restart workspace when ttl is changed #5391

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
Dec 21, 2022

Conversation

presleyp
Copy link
Contributor

Closes #5080

If ttl is changed, a dialog will show asking if you want the workspace to restart. If you choose yes, you will go to the workspace page and then see the restart process. This results in the new ttl taking effect.

@presleyp presleyp requested a review from a team as a code owner December 12, 2022 20:30
@presleyp presleyp requested review from Kira-Pilot and removed request for a team December 12, 2022 20:30
@presleyp presleyp requested a review from Kira-Pilot December 14, 2022 20:09
…t.tsx

Co-authored-by: Kira Pilot <kira@coder.com>
)
}
if (event.autoStopChanged) {
await API.putWorkspaceAutostop(context.workspace.id, event.ttl)
Copy link
Member

Choose a reason for hiding this comment

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

What happens if this errors? I don't see a catch, so I'm not sure how we'd enter the error state on line 157 of this machine. I would want to avoid prompting users to restart their workspace if no change was actually made.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if it was a problem that I'm not returning the result from both promises here, so I tried hard-coding each api call to return a rejected promise, and it behaved the way I intended: if one api call errors and that part of the schedule is not changed, there's no error. If the error api call is called, you stay on the form and get an error message. If you change the start but not the stop, you don't see the prompt.

actions: ["assignGetWorkspaceError"],
tags: "loading",
},
gettingPermissions: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Permissions are loaded in the initial app loading in the auth machine so I don't think we need to load it again. If there are new permissions, I would add them there.

I would do is something like this:

const permissions = usePermissions()
const [workspaceScheduleState] = useMachine(workspaceSchedule, { context: { permissions } })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I didn't add the permissions stuff in this PR (the machine just got reformatted), but I'll make a note to look into that later.

},
],
},
tags: "loading",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe, we could condense the gettingPermissions, gettingWorkspace, and gettingTemplate in a single state called loading + a load service that returns all this info. I'm thinking we are over-creating states making the machine more complex to understand without a clear value. By over-creating, I mean states that are not reflected in the UI/UX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, on one hand having separate states is helpful for debugging, but on the other hand it's extra states that aren't strictly necessary. We could change the way we handle errors to save whichever error we get going through the whole loading sequence. I'm not going to hold up this PR over it but it's worth considering in the future.

Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma left a comment

Choose a reason for hiding this comment

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

LGTM. I left a few comments but any of them are blockers. You can just skip them if they don't make sense or make it later, your call!

@presleyp presleyp merged commit 8d95285 into main Dec 21, 2022
@presleyp presleyp deleted the autostop-update/5080/presleyp branch December 21, 2022 15:11
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2022
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.

bug: "Stops at" UI element does not reflect the set auto-stop value
3 participants