- Notifications
You must be signed in to change notification settings - Fork921
fix!: stop workspace before update#18425
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
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
42420b1 tofa2d4ebCompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
No notes, awesome work@johnstcn 💪
This reverts commitc33f9b9.
ce577e1 toda8bd12Compare| @@ -1245,16 +1245,16 @@ func CreateWorkspace(t testing.TB, client *codersdk.Client, templateID uuid.UUID | |||
| } | |||
| // TransitionWorkspace is a convenience method for transitioning a workspace from one state to another. | |||
| func MustTransitionWorkspace(t testing.TB, client *codersdk.Client, workspaceID uuid.UUID, from, todatabase.WorkspaceTransition, muts ...func(req *codersdk.CreateWorkspaceBuildRequest)) codersdk.Workspace { | |||
| func MustTransitionWorkspace(t testing.TB, client *codersdk.Client, workspaceID uuid.UUID, from, tocodersdk.WorkspaceTransition, muts ...func(req *codersdk.CreateWorkspaceBuildRequest)) codersdk.Workspace { | |||
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.
review: This is annoying to have as a database type, so migrated to use the codersdk type instead. This inflated the diff a bit but removes an unnecessary conversion.
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.
The react code looks good
I took a bit of a closer look this time tho, and I'm wondering if this should be handled by the backend rather than the clients? The CLI and the frontend are now both responsible for producing this behavior correctly, and I worry about them getting out of sync or needing to add this in even more places, like the VSCode extension or the JetBrains plugin.
| @@ -2274,6 +2275,19 @@ class ApiMethods { | |||
| throw new MissingBuildParameters(missingParameters, activeVersionId); | |||
| } | |||
| // Stop the workspace if it is already running. | |||
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.
hmm. I'm a bit worried about adding this directly inside theAPI method. most everything in here is 1:1 with a backend endpoint, occasionally with minimal logic to convert from an options object to a query string or whatever. but this is actual application logic leaking in here.
Yeah, that's a valid concern. The main difficulty there is that there's no 'restart' transition in our current workspace transition model. Ideally we would have something like a 'restart' type provisioner job that would handle both the stop and start stages (or maybe perform a stop and enqueue a start build upon completion). I'd like us to do that eventually as it would greatly simplify the whole implementation here and let us model both restart and update as the same kind of state transition. Would you be OK with making this a follow-up item in the interest of expediency? |
I vote for later, but soon™️ |
Uh oh!
There was an error while loading.Please reload this page.
Fixes#17840
NOTE: calling this out as a breaking change so that it is highly visible in the changelog.
coder updateto stop the workspace if already running.