-
Notifications
You must be signed in to change notification settings - Fork 53
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
Add support for Github App Authentication #12
Conversation
Signed-off-by: Mihai Petracovici <[email protected]>
This is amazing! I will take some time this week to test it out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome. All these reviews are fairly minor because I am excited to get this in.
Thank you especially for documenting this change so well. Even if it is just adopted from elsewhere.
Signed-off-by: Mihai Petracovici <[email protected]>
Ok. I think I got everything you wanted. Let me know if I missed anything! |
One thing I'm noticing is your markdown link checker in your tests is failing on the placeholder links for creating the github app with the appropriate permissions. Is there a way to add exceptions to that? |
Signed-off-by: Mihai Petracovici <[email protected]>
Great! Thanks for the merge. |
With redhat-actions/openshift-actions-runners#12 Signed-off-by: Mihai Petracovici <[email protected]>
I've released |
Awesome! Thanks for all your help with reviews. |
thank you for the best unsolicited contribution we've had so far! :) |
Description
This PR adds support for using Github App authentication at either the Org level or Repository level. To avoid adding dependencies on another interpreted language, I opted to implement the JWT signing and token retrieval in shell based on Github's docs and some helpful stack overflow articles.
The documentation update is only a minor edit of (https://github.com/actions-runner-controller/actions-runner-controller#deploying-using-github-app-authentication). I wasn't exactly certain how to attribute that (I added a section to the Credits section of the README) so any advice there is welcome.
If this PR is accepted, I plan on making one to the Helm chart to support this as well.
Related Issue(s)
There is no related issue in this repository; however there is one in the Helm Chart repository (redhat-actions/openshift-actions-runner-chart#4).
I guess I should have opened an issue here as well. Hopefully it's not too onerous to have the discussion in the PR instead.
Checklist
Changes made
An overview of the changes:
GITHUB_APP_ID
,GITHUB_APP_INSTALL_ID
, andGITHUB_APP_PEM
environmental variablesQuestions
I had a question about why you are using /bin/sh in all your registration scripts since /bin/bash is available in the Fedora image the runner is based on. I was unable to use process redirection ( <() ) in my github app token function because that is a bash-specific feature but the workaround (dumping credentials to a tmp directory then removing it) is not ideal. Would switching to /bin/bash be reasonable?
Otherwise please let me know if I can improve anything with this implementation or if you think it's better to just use a python script instead of shell for the JWT generation ( I have a version of this locally with a python implementation too).