Skip to content

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

Closed
tkalus wants to merge 3 commits into
webfactory:masterfrom
tkalus-forks:fix/cleanup_consistency
Closed

fix: Add Cmd Default/Input support to cleanup#183
tkalus wants to merge 3 commits into
webfactory:masterfrom
tkalus-forks:fix/cleanup_consistency

Conversation

@tkalus

@tkalus tkalus commented May 23, 2023

Copy link
Copy Markdown

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

@mpdude mpdude left a comment

Copy link
Copy Markdown
Member

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

tkalus commented May 23, 2023

Copy link
Copy Markdown
Author

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

tkalus commented Jun 1, 2023

Copy link
Copy Markdown
Author

@mpdude Is there anything else blocking this?

@sdt

sdt commented Aug 16, 2023

Copy link
Copy Markdown

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
Copy Markdown

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!

@thazhemadam thazhemadam mentioned this pull request Feb 19, 2024
@tkalus
tkalus force-pushed the fix/cleanup_consistency branch from 19c9e1b to 6c290a5 Compare March 4, 2024 19:02
@tkalus

tkalus commented Mar 4, 2024

Copy link
Copy Markdown
Author

@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
Copy Markdown

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

@edelanghe-ledger

Copy link
Copy Markdown

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

@bvnp43

bvnp43 commented May 21, 2024

Copy link
Copy Markdown

pls merge this pr @mpdude

@jeromecoupe