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.

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

fix: Add Cmd Default/Input support to cleanup #183

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tkalus
Copy link

@tkalus tkalus commented May 23, 2023

Bring post action step into consistency with main for changes introduced in #154.

Without this change, sshAgentCmd is undefined when passed to execFileSync() during cleanup and post is never successful.

Post job cleanup consistently showing:

Stopping SSH agent
The "file" argument must be of type string. Received undefined
Error stopping the SSH agent, proceeding anyway

Also add entries to the CHANGELOG, detailing v0.8.0 and v0.9.0 releases.

Fixes: #208
Fixes: #211

Copy link
Member

@mpdude mpdude left a comment

Choose a reason for hiding this comment

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

Makes sense!

Could you please update dist/cleanup.js as well by running npm run build?

@tkalus
Copy link
Author

tkalus commented May 23, 2023

Makes sense!

Could you please update dist/cleanup.js as well by running npm run build?

Thanks for the gentle reminder @mpdude.

Updated via docker using:

$ docker run \
  --interactive \
  --rm \
  --tty \
  --volume ${PWD}:/var/task \
  --workdir /var/task \
  node:16-buster \
  sh -c 'yarn install && npm run build'

Please advise if the update looks incorrect or if you'd like me to use a different version of node or run a modified command.

@tkalus
Copy link
Author

tkalus commented Jun 1, 2023

@mpdude Is there anything else blocking this?

@sdt
Copy link

sdt commented Aug 16, 2023

I'd love to see this PR released. I've got self-hosted runners with tons of ssh-agent processes hanging around.

This fixes it nicely.

@thazhemadam
Copy link

Gentle bump on this PR being merged. This would really help with the cleanup of the ssh-agent processes running in the context of non-containerised environments!

@tkalus
Copy link
Author

tkalus commented Mar 4, 2024

@mpdude, I've rebased my branch from upstream and re-run/updated the dist/ folder.

$ docker run \
    --interactive \
    --rm \
    --tty \
    --volume ${PWD}:/var/task \
    --workdir /var/task \
    node:20-buster \
    sh -c 'npm install -g npm@10.5.0 && yarn install && NODE_OPTIONS=--openssl-legacy-provider npm run build'

Like others above, I'd love to get this into a release.

@sambanks
Copy link

Also would love to see this one in for our self hosted runners.

@edelanghe-ledger
Copy link

Hi guys, I would love to see this one merged also 🙏 🙏

@bvnp43
Copy link

bvnp43 commented May 21, 2024

pls merge this pr @mpdude

@jeromecoupe
Copy link

Have this warning in Post Loads private SSH key as well on Github hosted runners. If this PR fixes it, would love to see it merged

Stopping SSH agent
The "file" argument must be of type string. Received undefined
Error stopping the SSH agent, proceeding anyway

@bobanj
Copy link

bobanj commented Jul 10, 2024

y u no merge dis? 😄

@bpedman
Copy link

bpedman commented Aug 2, 2024

🙏 please merge this PR

@mark-pictor-csec
Copy link

@mpdude It'd be nice to see this merged. Anything the community can do to help?

Bring `post` actions step into consistency with `main` for changes
introduced in webfactory#154

Without this change, `sshAgentCmd` is undefined when passed to
`execFileSync()` during `cleanup` and `post` is never successful.
Actual command:
$ docker run \
    --interactive \
    --rm \
    --tty \
    --volume ${PWD}:/var/task \
    --workdir /var/task \
    node:20-buster \
    sh -c 'npm install -g npm@10.5.0 && yarn install && NODE_OPTIONS=--openssl-legacy-provider npm run build'
@sanderpick
Copy link

does anyone have a stable fork with this fix?

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.

changelog out of date Post cleanup fails