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

Parameterise execution of runner (update) #152

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

esteve
Copy link

@esteve esteve commented May 18, 2023

This is just #122 rebased on top of main and dist/index.js updated after running npm 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)

@esteve
Copy link
Author

esteve commented May 18, 2023

@kpturner I hope it's ok with you I submitted this PR, I don't mean to take credit for your work, I've submitted it in the hopes that it'd be easier for @machulav to merge your work more quickly (dist/index.js up to date, fix for runner running as a user).

@kpturner
Copy link

@kpturner I hope it's ok with you I submitted this PR, I don't mean to take credit for your work, I've submitted it in the hopes that it'd be easier for @machulav to merge your work more quickly (dist/index.js up to date, fix for runner running as a user).

It is fine by me - as long as it helps somebody I am not bothered about who gets the credit :)

@esteve esteve force-pushed the Optionally-execute-runner-as-a-service branch from e78e760 to 07e593a Compare July 12, 2023 10:03
@esteve
Copy link
Author

esteve commented Jul 12, 2023

@machulav I've rebased this PR on top of main to fix the conflicts. Is there anything that needs to be addressed before being merged? Thanks.

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`);
Copy link

Choose a reason for hiding this comment

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

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?

@@ -27,9 +29,18 @@ function buildUserDataScript(githubRegistrationToken, label) {
'tar xzf ./actions-runner-linux-${RUNNER_ARCH}-2.299.1.tar.gz',
Copy link

Choose a reason for hiding this comment

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

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.

@esteve
Copy link
Author

esteve commented Jul 19, 2023

@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!

@hvgazula
Copy link

Hello! Any chance this PR will be merged to enable running the ec2 self-hosted runner as non-root?

@esteve
Copy link
Author

esteve commented Feb 14, 2024

@machulav @hvgazula we've been using this branch for the past eight months at the Autoware Foundation without an issue, but I'd prefer it to get merged instead of using our fork. Is there any feedback I should address? Thanks.

@hvgazula
Copy link

@machulav @hvgazula we've been using this branch for the past eight months at the Autoware Foundation without an issue, but I'd prefer it to get merged instead of using our fork. Is there any feedback I should address? Thanks.

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?

@esteve
Copy link
Author

esteve commented Feb 14, 2024

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

@hvgazula
Copy link

Thanks, @esteve. It worked.

@thimios
Copy link

thimios commented Apr 16, 2024

@machulav we also need to run specs as a non-root user, it would be great to see this merged

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.

None yet

5 participants