add option to skip dependency installation #77
Conversation
seemethere
force-pushed
the
skip_dependency_installation
branch
from
b496958
to
7ba0f91
Compare
apt-get is not available on certain distributions so it's better to have the option to leave it out so we can still have the ability to run this action without having to be on ubuntu. Signed-off-by: Eli Uriegas <eliuriegas@fb.com>
seemethere
force-pushed
the
skip_dependency_installation
branch
from
7ba0f91
to
f708e7b
Compare
| @@ -54,7 +54,7 @@ describe('Tmate GitHub integration', () => { | |||
| Object.defineProperty(process, "platform", { | |||
| value: "linux" | |||
| }) | |||
| core.getInput.mockReturnValue("false") | |||
| core.getInput.mockReturnValueOnce("false").mockReturnValueOnce("true").mockReturnValue("false") | |||
There was a problem hiding this comment.
Why do we need two additional mocked return values? From looking at the code, I would have expected only one additional one, just like in the hunk above.
| @@ -63,6 +63,19 @@ describe('Tmate GitHub integration', () => { | |||
| expect(core.info).toHaveBeenNthCalledWith(2, `SSH: ${customConnectionString}`); | |||
| expect(core.info).toHaveBeenNthCalledWith(3, "Exiting debugging session because the continue file was created"); | |||
| }); | |||
| it('should be handle the main loop for linux without installing dependencies', async () => { | |||
There was a problem hiding this comment.
I have troubles parsing this description. Could you rephrase this?
| Object.defineProperty(process, "platform", { | ||
| value: "linux" | ||
| }) | ||
| core.getInput.mockReturnValue("false") |
There was a problem hiding this comment.
Why do we need only one mocked return value here? I would have expected more...
| default: 'false' |
There was a problem hiding this comment.
would be cool if you could revert the line-ending change from this file so the diff is clean.
| await execShellCommand(optionalSudoPrefix + 'apt-get install -y openssh-client xz-utils'); | ||
|
|
||
| if (core.getInput('install_dependencies') === "true") { | ||
| await installDependenciesLinux(optionalSudoPrefix); |
There was a problem hiding this comment.
There is now "duplicate code" which confuses me a little bit between the default parameter in the installDependenciesLinux function and the optionalSudoPrefix variable.
I think passing a boolean into the function if its sudo or passing the prefix directly into the function would be cleaner.
There was a problem hiding this comment.
Additionally, why even bother refactoring this into a separate function? Instead, the else could be turned into else if (core.getInput("install-dependencies") === "true"). Please note that I implicitly also addressed two more issues for consistency:
- we already use kebap-case in the action, not snake case (i.e.
-instead of_in the input name) - we seem to use double quotes rather consistently, not single quotes.
| @@ -116,4 +121,4 @@ function didTmateQuit() { | |||
| function continueFileExists() { | |||
| const continuePath = process.platform === "win32" ? "C:/msys64/continue" : "/continue" | |||
| return fs.existsSync(continuePath) || fs.existsSync(path.join(process.env.GITHUB_WORKSPACE, "continue")) | |||
| } | |||
There was a problem hiding this comment.
same: revert the line-ending change please.
|
@seemethere are you interested in continuing this Pull Request? otherwise we would close it, thanks! |
|
I think we can close this! I ended up approaching this problem from a different angle and no longer need this PR |
add option to skip dependency installation [#77 reopen]
apt-get is not available on certain distributions so it's better to have
the option to leave it out so we can still have the ability to run this
action without having to be on ubuntu.
Indirectly helps resolve #54