Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Start workspaces by shelling out to CLI #400

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+76 −37
Open

Start workspaces by shelling out to CLI #400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 43 additions & 15 deletions src/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { spawn, ChildProcessWithoutNullStreams } from "child_process"
import { Api } from "coder/site/src/api/api"
import { ProvisionerJobLog, Workspace } from "coder/site/src/api/typesGenerated"
import fs from "fs/promises"
Expand Down Expand Up @@ -122,29 +123,56 @@ export async function makeCoderSdk(baseUrl: string, token: string | undefined, s
/**
* Start or update a workspace and return the updated workspace.
*/
export async function startWorkspaceIfStoppedOrFailed(restClient: Api, workspace: Workspace): Promise<Workspace> {
// If the workspace requires the latest active template version, we should attempt
// to update that here.
// TODO: If param set changes, what do we do??
const versionID = workspace.template_require_active_version
? // Use the latest template version
workspace.template_active_version_id
: // Default to not updating the workspace if not required.
workspace.latest_build.template_version_id

export async function startWorkspaceIfStoppedOrFailed(
restClient: Api,
binPath: string,
workspace: Workspace,
writeEmitter: vscode.EventEmitter<string>,
): Promise<Workspace> {
// 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, [
Copy link
Member

Choose a reason for hiding this comment

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

Was there an issue with the types? I would expect this to have the same effect:

Suggested change
const startProcess: ChildProcessWithoutNullStreams = spawn(binPath, [
const startProcess = spawn(binPath, [

"start",
"--yes",
workspace.owner_name + "/" + workspace.name,
Copy link
Member

Choose a reason for hiding this comment

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

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.

])

startProcess.stdout.on("data", (data: Buffer) => {
data
.toString()
.split(/\r*\n/)
.forEach((line: string) => {
if (line !== "") {
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

@code-asher code-asher Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

writeEmitter.fire(line.toString() + "\r\n")
}
})
})

startProcess.stderr.on("data", (data: Buffer) => {
data
.toString()
.split(/\r*\n/)
.forEach((line: string) => {
if (line !== "") {
writeEmitter.fire(line.toString() + "\r\n")
}
})
})

return {
...updatedWorkspace,
latest_build: latestBuild,
}
startProcess.on("close", (code: number) => {
if (code === 0) {
resolve(restClient.getWorkspace(workspace.id))
} else {
reject(new Error(`"coder start" process exited with code ${code}`))
Copy link
Member

Choose a reason for hiding this comment

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

Thinking out loud, I was hitting this (because of the --global-config stuff) and wondering if we can make the error clearer. My thinking:

  1. Could possibly put the full command we are running, so I can copy-paste to try it myself.
  2. If we capture stderr separately 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!

}
})
})
}

/**
Expand Down
55 changes: 33 additions & 22 deletions src/remote.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,11 @@ export class Remote {
/**
* Try to get the workspace running. Return undefined if the user canceled.
*/
private async maybeWaitForRunning(restClient: Api, workspace: Workspace): Promise<Workspace | undefined> {
private async maybeWaitForRunning(
restClient: Api,
workspace: Workspace,
binPath: string,
): Promise<Workspace | undefined> {
// Maybe already running?
if (workspace.latest_build.status === "running") {
return workspace
Expand All @@ -63,6 +67,28 @@ export class Remote {
let terminal: undefined | vscode.Terminal
let attempts = 0

function initWriteEmitterAndTerminal(): vscode.EventEmitter<string> {
if (!writeEmitter) {
writeEmitter = new vscode.EventEmitter<string>()
}
if (!terminal) {
terminal = vscode.window.createTerminal({
name: "Build Log",
location: vscode.TerminalLocation.Panel,
// Spin makes this gear icon spin!
iconPath: new vscode.ThemeIcon("gear~spin"),
pty: {
onDidWrite: writeEmitter.event,
close: () => undefined,
open: () => undefined,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
} as Partial<vscode.Pseudoterminal> as any,
})
terminal.show(true)
}
return writeEmitter
}

try {
// Show a notification while we wait.
return await this.vscodeProposed.window.withProgress(
Expand All @@ -78,33 +104,17 @@ export class Remote {
case "pending":
case "starting":
case "stopping":
if (!writeEmitter) {
writeEmitter = new vscode.EventEmitter<string>()
}
if (!terminal) {
terminal = vscode.window.createTerminal({
name: "Build Log",
location: vscode.TerminalLocation.Panel,
// Spin makes this gear icon spin!
iconPath: new vscode.ThemeIcon("gear~spin"),
pty: {
onDidWrite: writeEmitter.event,
close: () => undefined,
open: () => undefined,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
} as Partial<vscode.Pseudoterminal> as any,
})
terminal.show(true)
}
writeEmitter = initWriteEmitterAndTerminal()
this.storage.writeToCoderOutputChannel(`Waiting for ${workspaceName}...`)
workspace = await waitForBuild(restClient, writeEmitter, workspace)
break
case "stopped":
if (!(await this.confirmStart(workspaceName))) {
return undefined
}
writeEmitter = initWriteEmitterAndTerminal()
this.storage.writeToCoderOutputChannel(`Starting ${workspaceName}...`)
workspace = await startWorkspaceIfStoppedOrFailed(restClient, workspace)
workspace = await startWorkspaceIfStoppedOrFailed(restClient, binPath, workspace, writeEmitter)
break
case "failed":
// On a first attempt, we will try starting a failed workspace
Expand All @@ -113,8 +123,9 @@ export class Remote {
if (!(await this.confirmStart(workspaceName))) {
return undefined
}
writeEmitter = initWriteEmitterAndTerminal()
this.storage.writeToCoderOutputChannel(`Starting ${workspaceName}...`)
workspace = await startWorkspaceIfStoppedOrFailed(restClient, workspace)
workspace = await startWorkspaceIfStoppedOrFailed(restClient, binPath, workspace, writeEmitter)
break
}
// Otherwise fall through and error.
Expand Down Expand Up @@ -292,7 +303,7 @@ export class Remote {
disposables.push(this.registerLabelFormatter(remoteAuthority, workspace.owner_name, workspace.name))

// If the workspace is not in a running state, try to get it running.
const updatedWorkspace = await this.maybeWaitForRunning(workspaceRestClient, workspace)
const updatedWorkspace = await this.maybeWaitForRunning(workspaceRestClient, workspace, binaryPath)
if (!updatedWorkspace) {
// User declined to start the workspace.
await this.closeRemote()
Expand Down