feat: support tokens scoped to multiple repositories within organization - #46
Conversation
There was a problem hiding this comment.
Thank you for the pull request!
I think there are two use cases we should support which are not covered by your code changes
- Create token for a given organization for all repositories
- Create token for current organization but for all repositories
For 1. if owner is set and repositories is not, then the token should have access across all repositories
For 2. we could recommend to set owner to env.GITHUB_REPOSITORY_OWNER (See environment variables)
With these use cases things will get a bit complicated and I think we should add tests for them. I can help with the tests if needed
|
@gr2m thanks for the feedback! Just pushed the changes you requested. Started to work on tests but my lack of time kept me from tests. So, I'll definitely take your offer to help with the tests! 🙇 👍 |
gr2m
left a comment
There was a problem hiding this comment.
did you try if it works as expected? You can update the build file and then use uses: timreimherr/create-github-app-token. I think if you don't set owner and repositories, then it will now create a token that has access to all repositories of the current repository owner's installation, instead of defaulting to just the current repository only
|
This is wonderful and extends the functionality of this action by quite a bit! 🎉 |
gr2m
left a comment
There was a problem hiding this comment.
I also updated the dist/main.cjs file
| // https://docs.github.com/en/rest/apps/apps?apiVersion=2022-11-28#get-a-repository-installation-for-the-authenticated-app | ||
| const { data: installation } = await request( | ||
| "GET /repos/{owner}/{repo}/installation", | ||
| "GET /orgs/{org}/installation", |
There was a problem hiding this comment.
if we need to get the installation ID for the current owner, we should keep doing it using GET /repos/{owner}/{repo}/installation. Because if the owner is set to something else, we need to check two endpoints, because the owner can be either an org or a user:
| { | ||
| owner, | ||
| repo, | ||
| org, |
There was a problem hiding this comment.
org is always empty here if owner2 is passed to this function
There was a problem hiding this comment.
The dist/* files are compiled build files. Better comment on the main.js and lib/main.js
I think that my PR here addresses your concern timreimherr#1
There was a problem hiding this comment.
This was an issue I ran into when trying to use your fork as an action. It might be that the dist file wasn't up to date at the time I made the comment, I just assumed it would be.
There was a problem hiding this comment.
ah makes sense, thank you for clarifying
left a comment
•
There was a problem hiding this comment.
still lacking tests but we can merge it as is now, I tested it manually. Waiting for review by @parkerbxyz to make sure I didn't miss anything
What I would love us to do as soon as this is merged is to create end-to-end tests to cover the different use cases for the new owner and repositories arguments, based off our existing end-to-end test:
create-github-app-token/.github/workflows/test.yml
Lines 25 to 49 in 5804f04
Co-authored-by: Parker Brown <17183625+parkerbxyz@users.noreply.github.com>
commented
Oct 4, 2023
|
🎉 This PR is included in version 1.3.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This adds support for a token to be scoped to multiple repositories within an organization.
resolves #4