Skip to content

Conversation

@umegaya
Copy link
Contributor

@umegaya umegaya commented Dec 2, 2021

hi, thank you for creating action-tmate! I really love its DX 👍

thie PR will add support for alpine linux, which does not have apt-get.
I added codes to detect linux distro via /etc/os-release and use apk if distro seems to be alpine.

this already runs fine on my github action, like https://github.com/suntomi/deplo/runs/4402074337?check_suite_focus=true#step:7:26

regards,

Copy link
Collaborator

@dscho dscho left a comment

Choose a reason for hiding this comment

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

This looks good, and I am delighted to see support for more Linux distributions!

I have just one request, see below.

src/helpers.js Outdated
* @return {Promise<string>}
*/
export const getLinuxDistro = () => {
const distro = execShellCommand("cat /etc/os-release | grep ^ID=").then(output => output.substring(3)/* trim 'ID=' */.trim());
Copy link
Collaborator

Choose a reason for hiding this comment

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

The existing code seems to prefer await/async over using Promises via .then(). Let's do the same.

Also, rather than spawning a full-blown shell and two commands to parse a file, it would make more sense to read the file contents and parse via string functions:

const getLinuxDistro = async () => {
  try {
    const osRelease = await fs.promises.readFile("/etc/os-release")
    const match = osRelease.toString().match(/^ID=(.*)$/m)
    return match ? match[1] : "(unknown)"
  } catch (e) {
    return "(unknown)"
  }
}

Copy link
Contributor Author

@umegaya umegaya Dec 3, 2021

Choose a reason for hiding this comment

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

@dscho agreed, I have fixed as you suggest. thanks for comment :D

core.debug("linux distro: [" + distro + "]");
if (distro === "alpine") {
// for set -e workaround, we need to install bash because alpine doesn't have it
await execShellCommand(optionalSudoPrefix + 'apk add openssh-client xz bash');
Copy link
Owner

Choose a reason for hiding this comment

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

is sudo a thing on alpine?

Copy link
Contributor Author

@umegaya umegaya Dec 3, 2021

Choose a reason for hiding this comment

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

@mxschmitt hmm, I think alpine container image (main use case of alpine) does not have it.

I try to fix it, but things bit complex because sudo will be required for invoking apk add sudo if user want to use sudo. so I think 2 options

  1. if sudo option is set for alpine, raise error like sudo option is not supported on alpine
  2. try invoking apk without optionalSudoPrefix for installing sudo on alpine (but it would not succeed the case user need sudo...)

personally I prefer option 1 because alpine's main use case is as container image, which should not require sudo.

how do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with c93ade2 I implement option 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think another option is enable sudo option but don't install sudo command by the action (cancel c93ade2).

some user may use non-root user for their alpine based image.

in this case, user can use the action if they install sudo on their image creation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

An even better method would be to use https://nodejs.org/api/process.html#processgeteuid to determine whether sudo is even needed. I bet that we're running as root in the Alpine container already.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, never mind, I just realized that we're not using sudo by default. Sorry for the noise.

@umegaya umegaya force-pushed the master branch 3 times, most recently from c93ade2 to a3d4598 Compare December 4, 2021 02:23
@dscho dscho merged commit b926bc4 into mxschmitt:master Dec 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants