-
Notifications
You must be signed in to change notification settings - Fork 737
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 GitHub#getInstallation to access endpoints related to the authenticated installation #1523
Conversation
Hey @bitwiseman , This one is kinda in the way of our work on Quarkus so we would appreciate a lot if you could review and cook a release once everything is fine. Thanks! |
…icated installation
74fd004
to
cc85d4a
Compare
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.
Thanks for working on this! It looks good to me in general. I think that adding the GHAuthenticatedAppInstallation
class is a good idea to avoid polluting the GitHub
class with methods that only work in some cases. Support for DELETE /installation/token
can easily be added to the class later, and perhaps GitHub will add other similar /installation
API endpoints that could live in the new class in the future.
(I am not a maintainer.)
*/ | ||
public class GHAuthenticatedAppInstallation extends GitHubInteractiveObject { | ||
protected GHAuthenticatedAppInstallation(@Nonnull GitHub root) { | ||
super(root); |
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.
Is it possible with the current codebase to detect whether the current GitHub client is authenticated as an installation when this object is instantiated on the client side? If so, maybe it would be nice to throw an exception with a helpful error message.
That said, do you know what the error is from GitHub when you call listRepositories with incorrect authentication? If it is already pretty clear, then there's probably no reason to do anything extra here.
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.
Is it possible with the current codebase to detect whether the current GitHub client is authenticated as an installation when this object is instantiated on the client side?
Judging from how GitHub#getApp()
fails with an obscure HTTP authentication error when we try to use it with an installation token, I would say there is no such feature yet. That would probably be a nice improvement in another PR that would be applied across the board to all relevant methods, not just this one? I think the hardest part would probably be testing...
That said, do you know what the error is from GitHub when you call listRepositories with incorrect authentication? If it is already pretty clear, then there's probably no reason to do anything extra here.
We get this, so I suppose it depends what you call "pretty clear":
org.kohsuke.github.HttpException: {"message":"Bad credentials","documentation_url":"https://docs.github.com/rest"}
at org.kohsuke.github.GitHubConnectorResponseErrorHandler$1.onError(GitHubConnectorResponseErrorHandler.java:56)
at org.kohsuke.github.GitHubClient.detectKnownErrors(GitHubClient.java:424)
at org.kohsuke.github.GitHubClient.sendRequest(GitHubClient.java:386)
at org.kohsuke.github.GitHubPageIterator.fetch(GitHubPageIterator.java:142)
at org.kohsuke.github.GitHubPageIterator.hasNext(GitHubPageIterator.java:89)
at org.kohsuke.github.PagedSearchIterable$1.hasNext(PagedSearchIterable.java:86)
at org.kohsuke.github.PagedIterator.fetch(PagedIterator.java:106)
at org.kohsuke.github.PagedIterator.nextPageArray(PagedIterator.java:134)
at org.kohsuke.github.PagedIterable.toArray(PagedIterable.java:78)
at org.kohsuke.github.PagedIterable.toArray(PagedIterable.java:106)
at org.kohsuke.github.PagedIterable.toList(PagedIterable.java:118)
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.
{"message":"Bad credentials","documentation_url":"https://docs.github.com/rest"}
Well that is true, but the message is a bit opaque 😄
Either way this is something that could easily be improved in the future if users are actually getting confused by it, so there's probably no reason to complicate this PR.
Codecov ReportBase: 78.95% // Head: 78.97% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1523 +/- ##
============================================
+ Coverage 78.95% 78.97% +0.01%
- Complexity 2129 2132 +3
============================================
Files 204 205 +1
Lines 6463 6469 +6
Branches 362 362
============================================
+ Hits 5103 5109 +6
Misses 1148 1148
Partials 212 212
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Hey @bitwiseman , thanks for allowing the CI build. It looks like the build passes, for this PR as well as for my two other PRs (#1524, #1525). I'll be happy to address any comment you have on those PRs. If the PR look good to you, do you think you could merge them and release a new version of github-api? As Guillaume mentioned earlier, we would need these patches in a Quarkus-related project :) |
@yrodiere |
Release coming soon. |
Thanks @bitwiseman! |
I was able to release by doing some hacking. Enjoy! |
Great to see that @bitwiseman , thanks! We're already using it in production :) |
Description
Fixes #1082.
Relevant documentation: https://docs.github.com/en/rest/apps/installations#list-repositories-accessible-to-the-app-installation
As you can see, this endpoint can only be reached with an installation token; but
GHAppInstallation
can only be obtained throughGHApp
with a JWT, so its methodlistRepositories
just cannot work, except with hacks (setRoot
, ...).That's the problem we're trying to solve.
This fix just adds a method to
GitHub
to obtain an object representing the current installation, which exposes alistRepositories
method.Original idea by @dwnusbaum (#1082 (comment)).
Before submitting a PR:
mvn -D enable-ci clean install site
locally. If this command doesn't succeed, your change will not pass CI.main
. You will create your PR from that branch.When creating a PR: