Skip to content
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.

Sign up for GitHub

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

feat: add custom working-directory support #157

Closed
wants to merge 1 commit into from
Closed

feat: add custom working-directory support #157

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
// @ts-check
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"
Comment on lines -2 to +8
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refrain from mixing in such unrelated changes, it makes reviewing the changes unnecessarily harder.


const TMATE_LINUX_VERSION = "2.4.0"

Expand All @@ -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');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new getInput() call is unexpected by the tests and therefore causes this test failure. Please adjust the tests to expect that call.

if (workingDirectoryInput) {
core.info(`Starting with custom working directory: ${workingDirectoryInput}`);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
const workingDirectory = !workingDirectoryInput ? undefined : workingDirectoryInput;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// move to custom working directory if set
if (workingDirectory) {
process.chdir(workingDirectory);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@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().

}

let tmateExecutable = "tmate"
if (core.getInput("install-dependencies") !== "false") {
core.debug("Installing dependencies")
Expand Down