add environment variables and working directory support to command exec #204
Conversation
|
commit: |
🐳 Docker Image PublishedFROM cloudflare/sandbox:0.0.0-pr-204-6608c47Version: You can use this Docker image with the preview package from this PR. |
|
(Sharing some thoughts here about Overall, I quite like the features this PR is adding and think this is an important one to clean up and get right. It's been fragile for a long time and I'm happy we're fixing this! So with env, I generally want us to make sure there are 4 layers of env setting and all of them work cohesively:
And env is mainly critical for all the flows that involve the default session and the ones created explicitly using cwd is much easier to reason with as an option, and is definitely quite useful when working in the scope of either the default session or the manually created session, but sometimes developers just want to run a command elsewhere and don't want to do |
ghostwriternr
left a comment
ghostwriternr
left a comment
There was a problem hiding this comment.
Besides these (commandWithEnv is the only blocking one though), I think we could do a couple of things more:
- Add
setEnvVarsto theISandboxtype. I just realised this is not there currently, but should be. - In
createSession, IF you think my earlier comment aboutsetEnvVarsbehaviour on the global sandbox object vs the session object makes sense, we can mergethis.envVarswith the session-specific ones before creating a session from the session object.
| return `export ${key}='${escapedValue}'`; | ||
| }) | ||
| .join('; '); | ||
| // Wrap in subshell to isolate env vars (they don't persist in session) | ||
| commandWithEnv = `(${exports}; ${command})`; |
There was a problem hiding this comment.
There's probably a silent bug here, because if the command is a cd, it won't get persisted because we're running in a sub-shell. Probably a cleaner option is to remove the export bit, and change commandWithEnv to ${envPrefix} ${command};? This is a standard shell pattern to keep variables scoped to just that command.
| } else { | ||
| // Regular execution with session | ||
| const response = await this.client.commands.execute(command, sessionId); | ||
| const response = await this.client.commands.execute( |
There was a problem hiding this comment.
Minor nitpick, okay to skip - since this is an internal method, we can probably make options an object instead of extending the number of params.
| processId: body.processId, | ||
| env: body.env, | ||
| cwd: body.cwd, | ||
| timeout: body.timeout, | ||
| autoCleanup: body.autoCleanup |
There was a problem hiding this comment.
Are the rest of these fields actually being used properly? I would say we should not add them to the scope of this PR, but if not addressed, we should create a separate issue to track. #81 was created a while ago for the timeout.
| @@ -0,0 +1,195 @@ | |||
| import { | |||
There was a problem hiding this comment.
Is this test intended to be checked-in? Seems to be a mix of testing pre-change behaviour vs new
2 participants
No description provided.