Refactor subprocess execution to use @actions/exec
#171
Conversation
| @@ -113,62 +99,65 @@ async function execCommands(commands: string[], cmdType: string) { | |||
|
|
|||
| startGroup(`🚀 Running ${cmdType}Commands`); | |||
| try { | |||
| const arrPromises = commands.map(async (command) => { | |||
| for (const command of commands) { | |||
There was a problem hiding this comment.
These commands were running in parallel, now sequentially. Is that intentional? Is that a breaking change?
There was a problem hiding this comment.
Yes, this is intentional, though I forgot to include a changeset for it. The previous behavior ought to be considered a bug, since ostensibly no one expects these to run in parallel and it could introduce race conditions. I don’t think this is a breaking change, but it would be had we gone from sequential to parallel execution.
src/index.ts
Outdated
| command = command.concat(` --env ${environment}`); | ||
| } | ||
| for (let command of commands) { | ||
| const args = ["wrangler", ...command.split(/\s+/)]; |
There was a problem hiding this comment.
This is hoping the command has no quotes strings with spaces
We might want to document this to also allow command to be an array as an escape hatch
There was a problem hiding this comment.
I think @actions/exec handles that scenario for us with its escaping logic, but worth looking into to confirm that.
There was a problem hiding this comment.
you were right, great catch. I've removed the split here and instead now pass the provided command (prepended with npx wrangler ) as part of the first argument to exec(), which is expected to be a string that has already been escaped. The second array argument is now used to provide any additional options we want to inject into the users command (e.g. --env ... or --vars ...), which exec() will escape for us.
Instead of using a mix of `child_process.exec`, `child_process.execSync` and a promisified version of `child_process.exec`, we now (mostly) just use `@actions/exec`. That runs `child_process.spawn` under the hood and handles a lot of character escaping for us. We can also now pass Buffers directly into the subprocess as stdin instead of relying on shell piping. This ends up fixing a few problems we had where secrets and env var values containing shell metacharacters were being misinterpreted. Unfortunately, `@actions/exec` doesn't support running with a shell. That means we still have to roll our own wrapper around `child_process.exec` to avoid a breaking change to `preCommands` and `postCommands`, since users might be expecting these to run within a shell. Also worth noting that we're no longer hiding stdout and stderr from the secret uploading step. We were previously doing this out of an abundance of caution, but it made debugging issues very difficult if secret upload failed for some reason. I feel ok doing this since we're no longer echoing & piping the secret values, wrangler doesn't ever output secret values, and as a last line of defense GitHub masks any secret values that accidentally get logged.
1000hz
force-pushed
the
cina/refactor-subprocess-exec
branch
from
b829c97
to
76d614f
Compare
|
Also fixes #200 |
Instead of using a mix of
child_process.exec,child_process.execSyncand a promisified version ofchild_process.exec, we now (mostly) just use@actions/exec. That runschild_process.spawnunder the hood and handles a lot of character escaping for us. We can also now pass Buffers directly into the subprocess as stdin instead of relying on shell piping.This ends up fixing a few problems we had where secrets and env var values containing shell metacharacters were being misinterpreted.
Unfortunately,
@actions/execdoesn't support running with a shell. That means we still have to roll our own wrapper aroundchild_process.execto avoid a breaking change topreCommandsandpostCommands, since users might be expecting these to run within a shell.Also worth noting that we're no longer hiding stdout and stderr during the secret uploading step. We were previously doing this out of an abundance of caution, but it made debugging issues very difficult if secret upload failed for some reason. I feel ok doing this since we're no longer echoing & piping the secret values, wrangler doesn't ever output secret values, and as a last line of defense GitHub masks any secret values that accidentally get logged.
Fixes #168