feat: Add enterprise app installation lookup#4230
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4230 +/- ##
=======================================
Coverage 97.45% 97.45%
=======================================
Files 189 189
Lines 19152 19154 +2
=======================================
+ Hits 18665 18667 +2
Misses 270 270
Partials 217 217 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| return s.getInstallation(ctx, fmt.Sprintf("orgs/%v/installation", org)) | ||
| } | ||
|
|
||
| // FindEnterpriseInstallation finds the enterprise's installation information. |
There was a problem hiding this comment.
The GitHub API docs call this "Get an enterprise installation for the authenticated app" so instead of "FindEnterpriseInstallation", can we use "GetEnterpriseInstallation"?
Wow... I see all the other ones in this file are called "Find*"... I'm not sure how I let that one slide by in code reviews. 😞
OK, let's take this to a vote from our dedicated team of reviewers... which do you like best?
- Option A: Just name this one endpoint "GetEnterpriseInstallation"
- Option B: Leave this one endpoint "FindEnterpriseInstallation"
- Option C: Rename (breaking API change) ALL methods whose documentation URL starts with "get-" to "Get*" instead of "Find*" in this file
- Option D: Something else?
cc: @stevehipwell - @alexandear - @zyfy29 - @Not-Dhananjay-Mishra - @munlicode
There was a problem hiding this comment.
I vote for Option C.
Because in my opinion, method needs to be consistent with GitHub API docs.
There was a problem hiding this comment.
Yeah, I agree Option C would be the cleanest long-term naming direction if the API is ever revisited more broadly.
For this PR, I used FindEnterpriseInstallation to stay consistent with the existing installation lookup methods in this file and avoid making a breaking API change as part of a small endpoint addition.
Let's wait for the other maintainers' votes as well.
There was a problem hiding this comment.
I vote for "Option B".
And "Option C" for the future PRs.
|
|
||
| // FindEnterpriseInstallation finds the enterprise's installation information. | ||
| // | ||
| // GitHub API docs: https://docs.github.com/enterprise-cloud@latest/rest/apps/apps#get-an-enterprise-installation-for-the-authenticated-app |
There was a problem hiding this comment.
| // GitHub API docs: https://docs.github.com/enterprise-cloud@latest/rest/apps/apps#get-an-enterprise-installation-for-the-authenticated-app | |
| // GitHub API docs: https://docs.github.com/enterprise-cloud@latest/rest/apps/apps?apiVersion=2022-11-28#get-an-enterprise-installation-for-the-authenticated-app |
Because s.getInstallation works with the API version 2022-11-18.
Summary
Adds support for looking up a GitHub App installation for an enterprise.
This implements the new
AppsService.FindEnterpriseInstallationhelper for:The method follows the existing app installation lookup pattern already used for organization, repository, and user installations.
Details
FindEnterpriseInstallation(ctx, enterprise)toAppsServiceGET /enterprises/{enterprise}/installationInstallationmodelTesting
All tests passed locally.
Fixes #4212