Parameterise execution of runner (update) #152
Conversation
It is fine by me - as long as it helps somebody I am not bothered about who gets the credit :) |
esteve
force-pushed
the
Optionally-execute-runner-as-a-service
branch
from
e78e760 to
07e593a
Compare
|
@machulav I've rebased this PR on top of |
| userData.push(`./svc.sh install ${config.input.runAsUser || ''}`); | ||
| userData.push('./svc.sh start'); | ||
| } else { | ||
| userData.push(`${config.input.runAsUser ? `su ${config.input.runAsUser} -c` : ''} ./run.sh`); |
There was a problem hiding this comment.
if run.sh supports only running as root then why do we run it under regular user? and if it supported the description of run-runner-as-user parameter should be updated.
and if this is possible why do we need to run it as a service?
also, won't it better to create the user within this script? also add it to sudoers to be closer to github environment?
src/aws.js
Outdated
| @@ -27,9 +29,18 @@ function buildUserDataScript(githubRegistrationToken, label) { | |||
| 'tar xzf ./actions-runner-linux-${RUNNER_ARCH}-2.299.1.tar.gz', | |||
There was a problem hiding this comment.
we should probably have a parameter/config to specify in which directory we want to extract and run the runner, optionally take some default from the image so the image may specify that.
setting this directory will allow to run the runner on differnt volume than the rootfs, which is large enough to run the job.
|
@alonbl I know nothing about Javascript, I only updated #122 and fixed a bug to make it easier for @machulav to merge this PR as we're currently using at the Autoware Foundation, but if there's any improvements missing, feel free to submit a PR targeting this one, I'm more than happy to incorporate your changes. Thanks! |
|
Hello! Any chance this PR will be merged to enable running the ec2 self-hosted runner as non-root? |
Thank you for replying @esteve. Is it on the main branch of your fork? I agree it'd be nice to have this feature merged in here. I'll ask this again for attention :). @machulav any chance you can merge this feature? |
|
@hvgazula it's here https://github.com/esteve/ec2-github-runner/tree/Optionally-execute-runner-as-a-service My fork is just an updated version of @kpturner 's work from #122 plus a couple of minor fixes needed to run as a non-root user. |
|
Thanks, @esteve. It worked. |
|
@machulav we also need to run specs as a non-root user, it would be great to see this merged |
|
@Preen sorry, I somehow missed your comment from months ago, I'll update the PR. Thanks. |
esteve
force-pushed
the
Optionally-execute-runner-as-a-service
branch
from
07e593a to
e4f971f
Compare
esteve
force-pushed
the
Optionally-execute-runner-as-a-service
branch
from
e4f971f to
a36f21d
Compare
|
@Preen I've fixed the conflicts and ran |
|
@esteve Thanks for keeping up on this one. I'd love to see this merged into this repo so I can go back to using the main upstream. FYI, I was pointing at your branch and the recent rebase seems to have borken things (I opened a PR to your fork just to highlight it). |
|
@lifeisafractal I've just merged your changes, thanks! 🙂 |
|
@lifeisafractal I ran docker run -e NODE_OPTIONS=--openssl-legacy-provider -it --rm -v `pwd`:`pwd` -w `pwd` node sh -c "npm run package"I'm working on adding an option to set the root volume size in another branch, but after running |
|
@lifeisafractal looks like I had a |
|
@esteve I'm not far (if at all) ahead of you with Node. I did a fresh clone with a |
|
Tested successfully, merged! |
This is just #122 rebased on top of
mainanddist/index.jsupdated after runningnpm run package, all credit goes to @kpturner.I've also fixed a small issue where if the CI job was set to run as a user, and let it download the GitHub runner code, it wouldn't work (see #122 (comment) for context, fix is in 35580d0)