feat: add custom working-directory support #157
Conversation
Signed-off-by: Rui Chen <[email protected]>
There was a problem hiding this comment.
Thank you for working on this feature request.
I point out a couple of things below. In addition, the way this Action is used requires npm run build to be called and the result to be committed. That's because GitHub Actions do not have a separate build step, their clones need to contain the fully built Action (including all of the dependencies).
| import os from "os" | ||
| import fs from "fs" | ||
| import path from "path" | ||
| import * as core from "@actions/core" | ||
| import * as github from "@actions/github" | ||
| import * as tc from "@actions/tool-cache" | ||
| import { Octokit } from "@octokit/rest" | ||
| import fs from "fs" | ||
| import os from "os" | ||
| import path from "path" |
There was a problem hiding this comment.
Please refrain from mixing in such unrelated changes, it makes reviewing the changes unnecessarily harder.
| import { execShellCommand, getValidatedInput, getLinuxDistro, useSudoPrefix } from "./helpers" | ||
| import { execShellCommand, getLinuxDistro, getValidatedInput, useSudoPrefix } from "./helpers" |
There was a problem hiding this comment.
Please refrain from mixing in such unrelated changes, it makes reviewing the changes unnecessarily harder.
| if (workingDirectoryInput) { | ||
| core.info(`Starting with custom working directory: ${workingDirectoryInput}`); | ||
| } | ||
| const workingDirectory = !workingDirectoryInput ? undefined : workingDirectoryInput; |
There was a problem hiding this comment.
How about naming the variable workingDirectory instead of workingDirectoryInput in the first place? There is no need for two variables when a single one can do the job just as well, or even better.
| // custom working directory | ||
| const workingDirectoryInput = core.getInput('working-directory'); | ||
| if (workingDirectoryInput) { | ||
| core.info(`Starting with custom working directory: ${workingDirectoryInput}`); |
There was a problem hiding this comment.
This new core.info() is unexpected by the tests and therefore causes this failure. I would suggest to either drop the core.info() call, or to adjust the tests.
| @@ -26,6 +26,18 @@ const sleep = (ms) => new Promise(resolve => setTimeout(resolve, ms)); | |||
|
|
|||
| export async function run() { | |||
| try { | |||
| // custom working directory | |||
| const workingDirectoryInput = core.getInput('working-directory'); | |||
There was a problem hiding this comment.
This new getInput() call is unexpected by the tests and therefore causes this test failure. Please adjust the tests to expect that call.
|
|
||
| // move to custom working directory if set | ||
| if (workingDirectory) { | ||
| process.chdir(workingDirectory); |
There was a problem hiding this comment.
Something seems to be wrong with this call because it breaks the tests.
The problem may be caused by process.chdir() making the main function non-reentrant: the first time it is called with a relative path, it might work, but then the next call will fail because it already changed the working directory to that path.
A better idea may be to teach the execShellCommand() function to accept an (optionally undefined) cwd parameter that is passed to the spawn() calls, and then specify workingDirectory as that cwd parameter in the new-session call. That way, the change of the working directory is limited to the call that actually needs it.
There was a problem hiding this comment.
A better idea may be to teach the
execShellCommand()function to accept an (optionally undefined)cwdparameter that is passed to thespawn()calls, and then specifyworkingDirectoryas thatcwdparameter in thenew-sessioncall.
@chenrui333 This should be much easier now: I already introduced that options parameter in dcd27aa, and all you need to do is to piggyback on that code by adding cwd: options && options.cwd to the options that are passed to spawn().
|
@mxschmitt thanks for the review, I will check on them later. |
fixes #141