Start workspaces by shelling out to CLI #400
Conversation
Replace the REST-API-based start flow with one that shells out to the coder CLI. Signed-off-by: Aaron Lehmann <alehmann@netflix.com>
aaronlehmann
force-pushed
the
start-with-cli
branch
from
f8319a8
to
59ac05c
Compare
| // Before we start a workspace, we make an initial request to check it's not already started | ||
| const updatedWorkspace = await restClient.getWorkspace(workspace.id) | ||
|
|
||
| if (!["stopped", "failed"].includes(updatedWorkspace.latest_build.status)) { | ||
| return updatedWorkspace | ||
| } | ||
|
|
||
| const latestBuild = await restClient.startWorkspace(updatedWorkspace.id, versionID) | ||
| return new Promise((resolve, reject) => { | ||
| const startProcess: ChildProcessWithoutNullStreams = spawn(binPath, [ |
There was a problem hiding this comment.
Was there an issue with the types? I would expect this to have the same effect:
| const startProcess: ChildProcessWithoutNullStreams = spawn(binPath, [ | |
| const startProcess = spawn(binPath, [ |
| const startProcess: ChildProcessWithoutNullStreams = spawn(binPath, [ | ||
| "start", | ||
| "--yes", | ||
| workspace.owner_name + "/" + workspace.name, |
There was a problem hiding this comment.
This will be the slightly tricky part I think. For most folks this will also need --global-config so the cli can pick up the url and token.
--global-config expects url and session files in the directory to which it points, so it may be slightly tricky because the vscodessh command that the plugin has used up to this point takes separate url and file config flags (not sure why), and I guess as a consequence the token file (perhaps unintentionally) ended up with the slightly different name session_token (again not sure why the difference).
So all that to say, I think we will have to also make sure the session file exists, either by migrating from session_token to session or writing both, and then add the --global-config flag.
| if (code === 0) { | ||
| resolve(restClient.getWorkspace(workspace.id)) | ||
| } else { | ||
| reject(new Error(`"coder start" process exited with code ${code}`)) |
There was a problem hiding this comment.
Thinking out loud, I was hitting this (because of the --global-config stuff) and wondering if we can make the error clearer. My thinking:
- Could possibly put the full command we are running, so I can copy-paste to try it myself.
- If we capture
stderrseparately perhaps we could add it to this error. Or, if we avoid disposing the terminal until later (or maybe we could just not dispose it at all, could be nice to pull up the build log whenever you want instead of having it immediately vanish) so it can still be viewed while the popup is up, that could work!
| .toString() | ||
| .split(/\r*\n/) | ||
| .forEach((line: string) => { | ||
| if (line !== "") { |
There was a problem hiding this comment.
Are there a lot of blank lines we have to strip? Does the API strip these out for us maybe so this is for parity?
Not a big deal, my only thought is that maybe start uses blank lines to delineate sections for readability or something (no idea though).
There was a problem hiding this comment.
I did see blank lines here, I believe due to the CLI's use of \r to overwrite existing lines (which doesn't work if passed through directly to VS Code's output).
There was a problem hiding this comment.
Aahhh right, I completely forgot it does that. That makes perfect sense. Since we display this in a (fake) terminal I wonder if we can pass through directly in a way that handles that case so it looks the same as when you run it directly on the command line, but this looks great to me for now.
|
Thanks for reviewing! I'm traveling right now but will come back to this in early December. |
Replace the REST-API-based start flow with one that shells out to the coder CLI.