Add support for enterprise level GitHub Apps#263
Add support for enterprise level GitHub Apps#263theztefan wants to merge 28 commits intoactions:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request adds support for generating GitHub App tokens for enterprise-level installations, allowing GitHub Apps to authenticate with enterprise management APIs. The implementation includes comprehensive input validation to ensure mutual exclusivity with existing repository-scoped functionality.
Key changes include:
- Added
enterpriseinput parameter with validation for mutual exclusivity withownerandrepositories - Implemented enterprise installation discovery by listing all installations and filtering for enterprise type
- Added comprehensive test coverage for success scenarios, error cases, and input validation
Reviewed Changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| action.yml | Added enterprise input definition |
| main.js | Added enterprise parameter reading and passing |
| lib/main.js | Core logic for enterprise token generation and validation |
| package.json | Version bump to 2.0.7 |
| README.md | Documentation for enterprise usage |
| tests/*.js | Comprehensive test suite for enterprise functionality |
| tests/snapshots/index.js.md | Test output snapshots for verification |
Comments suppressed due to low confidence (1)
Co-authored-by: Parker Brown <17183625+parkerbxyz@users.noreply.github.com>
Co-authored-by: Parker Brown <17183625+parkerbxyz@users.noreply.github.com>
Co-authored-by: Parker Brown <17183625+parkerbxyz@users.noreply.github.com>
Co-authored-by: Parker Brown <17183625+parkerbxyz@users.noreply.github.com>
Co-authored-by: Parker Brown <17183625+parkerbxyz@users.noreply.github.com>
Co-authored-by: Parker Brown <17183625+parkerbxyz@users.noreply.github.com>
|
Thanks for this! I'm opening an issue to add the required API for this, since it is a clear gap. No ETA on the resolution I'm afraid. |
|
Something we might be able to do while we await a dedicated API:
This should allow people to use this action with enterprise-level GitHub Apps. |
|
Is there any update on using this experimental injection of the installation ID? At the very least, it would allow us to use it in the meantime, and we can switch over once a proper API is available. Since the installation ID won't change often for most users, hardcoding it wouldn't be a major issue. |
|
Hey all, |
|
@hpsin any updates? |
|
It should get announced today, and should be live on the API docs now - https://docs.github.com/en/enterprise-cloud@latest/rest/apps/apps?apiVersion=2022-11-28#get-an-enterprise-installation-for-the-authenticated-app |
|
@hpsin any chance to get this one merged/released? |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| // Set up environment with enterprise and owner set | ||
| for (const [key, value] of Object.entries(DEFAULT_ENV)) { | ||
| process.env[key] = value; | ||
| } | ||
| process.env.INPUT_ENTERPRISE = "test-enterprise"; | ||
| process.env.INPUT_OWNER = "test-owner"; | ||
|
|
||
| await import("../main.js"); | ||
| } catch (error) { | ||
| console.error(error.message); | ||
| } |
There was a problem hiding this comment.
This test wraps await import("../main.js") in a try/catch, but main.js already catches errors internally and sets a failing exit code via core.setFailed, so the catch block never runs. Removing the try/catch makes the test’s intent clearer and avoids accidentally swallowing failures if main.js error handling changes in the future.
| try { | |
| // Set up environment with enterprise and owner set | |
| for (const [key, value] of Object.entries(DEFAULT_ENV)) { | |
| process.env[key] = value; | |
| } | |
| process.env.INPUT_ENTERPRISE = "test-enterprise"; | |
| process.env.INPUT_OWNER = "test-owner"; | |
| await import("../main.js"); | |
| } catch (error) { | |
| console.error(error.message); | |
| } | |
| // Set up environment with enterprise and owner set | |
| for (const [key, value] of Object.entries(DEFAULT_ENV)) { | |
| process.env[key] = value; | |
| } | |
| process.env.INPUT_ENTERPRISE = "test-enterprise"; | |
| process.env.INPUT_OWNER = "test-owner"; | |
| await import("../main.js"); |
| try { | ||
| // Set up environment with enterprise and repositories set | ||
| for (const [key, value] of Object.entries(DEFAULT_ENV)) { | ||
| process.env[key] = value; | ||
| } | ||
| process.env.INPUT_ENTERPRISE = "test-enterprise"; | ||
| process.env.INPUT_REPOSITORIES = "repo1,repo2"; | ||
|
|
||
| await import("../main.js"); | ||
| } catch (error) { | ||
| console.error(error.message); | ||
| } |
There was a problem hiding this comment.
This test wraps await import("../main.js") in a try/catch, but main.js already catches errors internally and sets a failing exit code via core.setFailed, so the catch block never runs. Removing the try/catch makes the test’s intent clearer and avoids accidentally swallowing failures if main.js error handling changes in the future.
| try { | |
| // Set up environment with enterprise and repositories set | |
| for (const [key, value] of Object.entries(DEFAULT_ENV)) { | |
| process.env[key] = value; | |
| } | |
| process.env.INPUT_ENTERPRISE = "test-enterprise"; | |
| process.env.INPUT_REPOSITORIES = "repo1,repo2"; | |
| await import("../main.js"); | |
| } catch (error) { | |
| console.error(error.message); | |
| } | |
| // Set up environment with enterprise and repositories set | |
| for (const [key, value] of Object.entries(DEFAULT_ENV)) { | |
| process.env[key] = value; | |
| } | |
| process.env.INPUT_ENTERPRISE = "test-enterprise"; | |
| process.env.INPUT_REPOSITORIES = "repo1,repo2"; | |
| await import("../main.js"); |
| enterprise: | ||
| description: "The slug version of the enterprise name for enterprise-level app installations (cannot be used with 'owner' or 'repositories')" | ||
| required: false |
There was a problem hiding this comment.
PR description mentions adding a new enterprise-slug input, but the actual input added/used is named enterprise (and main.js/tests/README align to that). Please update the PR description (or rename the input) so users aren’t confused about the correct input name.
This pull request adds support for generating GitHub App installation tokens for enterprise-level installations.
What changed
enterprise-sluginput toaction.yml.enterprise-slugthroughmain.jsandlib/main.js.enterprise-slugcannot be combined withownerorrepositories.GET /enterprises/{enterprise}/installation, then used the returned installation ID to mint an installation token through@octokit/auth-app.README.mdwith enterprise-installation usage and input documentation.Tests
Added focused test coverage for:
ownerrepositoriesThe test suite was also trimmed to remove redundant enterprise scenarios.
Notes
dist/changes are intentionally not included in this PR.Refs: