-
Notifications
You must be signed in to change notification settings - Fork 0
Start dev server in container after copying repo #1375
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
base: main
Are you sure you want to change the base?
Changes from all commits
080375f
a100b14
5d05b7d
7f4dbe4
c9c2118
9b162c1
c642746
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -366,3 +366,42 @@ export async function copyRepoToExistingContainer({ | |
| throw new Error(`Failed to copy repository to container: ${e}`) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Start a dev server inside an existing container for the app in `mountPath`. | ||
| * This determines the appropriate package manager based on lockfiles and then | ||
| * runs install and dev in the background using nohup so this call can return | ||
| * immediately. Logs are written to .dev.log and PID to .dev.pid under mountPath. | ||
| */ | ||
| export async function startDevServerInContainer({ | ||
| containerName, | ||
| mountPath = "/workspace", | ||
| }: { | ||
| containerName: string | ||
| mountPath?: string | ||
| }): Promise<{ stdout: string; stderr: string; exitCode: number }> { | ||
| // Build a shell script that: | ||
| // - chooses package manager (pnpm > yarn > npm) | ||
| // - runs install (preferring frozen/ci if lockfile present) | ||
| // - starts dev server | ||
| // - backgrounds the process with nohup and stores logs/pid | ||
| const script = [ | ||
| "set -e", | ||
| `cd ${mountPath}`, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Unquoted shell path breaks on paths with spacesThe Additional Locations (1) |
||
| // Pick package manager | ||
| "if [ -f pnpm-lock.yaml ]; then PM=pnpm; elif [ -f yarn.lock ]; then PM=yarn; else PM=npm; fi", | ||
| // Construct install command | ||
| 'if [ "$PM" = "pnpm" ]; then INSTALL="pnpm install --frozen-lockfile || pnpm install"; ' + | ||
| 'elif [ "$PM" = "yarn" ]; then INSTALL="yarn install --frozen-lockfile || yarn install"; ' + | ||
| 'else INSTALL="if [ -f package-lock.json ]; then npm ci || npm i; else npm i; fi"; fi', | ||
| // Start dev in background after install completes | ||
| 'nohup sh -c "$INSTALL && "$PM" run -s dev" > .dev.log 2>&1 & echo $! > .dev.pid', | ||
| 'echo "Dev server starting with $PM. Logs: .dev.log, PID: $(cat .dev.pid)"', | ||
| ].join(" && ") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Broken quoting prevents dev server startThe Additional Locations (1) |
||
|
|
||
| return await execInContainerWithDockerode({ | ||
| name: containerName, | ||
| command: script, | ||
| cwd: mountPath, | ||
| }) | ||
| } | ||
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.
Bug: Exit code ignored, failures return success incorrectly
The try-catch wrapping
startDevServerInContainerwon't catch script execution failures becauseexecInContainerWithDockerodereturns{ stdout, stderr, exitCode }instead of throwing exceptions on non-zero exit codes. The PR description states this endpoint should return HTTP 202 when dev server startup fails, but the returnedexitCodeis never checked. The endpoint will return HTTP 200 with{ success: true }even when the shell script fails. Other callers ofexecInContainerWithDockerodein the codebase properly checkexitCode === 0to determine success.