Support GitHubAppCredentials owner inference when specified on a pipeline using GitHub SCM#396
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #396 +/- ##
============================================
- Coverage 71.80% 71.77% -0.04%
- Complexity 162 165 +3
============================================
Files 16 16
Lines 532 542 +10
Branches 51 52 +1
============================================
+ Hits 382 389 +7
- Misses 124 125 +1
- Partials 26 28 +2 ☔ View full report in Codecov by Sentry. |
| apiUri = gitHubAppCredentials.getApiUri(); | ||
| if (context instanceof GitHubSCMSourceChecksContext) { | ||
| final var gitHubSCMSourceChecksContext = (GitHubSCMSourceChecksContext) context; | ||
| credentials = gitHubAppCredentials.withOwner(gitHubSCMSourceChecksContext.getOwner()); |
There was a problem hiding this comment.
I think you could add a test to
In the existing test the owner is set on the SCM context but not on the credentials
| private final String gitHubUrl; | ||
|
|
||
| @Nullable | ||
| private StandardUsernameCredentials credentials; |
There was a problem hiding this comment.
credentials where moved from method variable to class field. This helps GitHubChecksPublisherITest to check that owner is propagated from context to credentials.
timja
changed the title
| final var gitHubAppCredentials = (GitHubAppCredentials) credentials; | ||
| if (context instanceof GitHubSCMSourceChecksContext) { | ||
| final var gitHubSCMSourceChecksContext = (GitHubSCMSourceChecksContext) context; | ||
| credentials = gitHubAppCredentials.withOwner(gitHubSCMSourceChecksContext.getOwner()); |
There was a problem hiding this comment.
FWIW, I think it is undesirable to have the withOwner method be considered a public API of the plugin. Is there a reason we cannot use Connector.lookupScanCredentials from github-branch-source instead to handle contextualization automatically? It looks like it would require some API changes to SCMFacade methods to have them accept an owner parameter, but would that be ok, or is there some fundamental problem with it?
There was a problem hiding this comment.
Sounds fine if someone wants to work on it.
There was a problem hiding this comment.
I'll create a follow-up PR ASAP.
3 participants
GitHub App installed in multiple organizations get:
Steps to reproduce:
This fixes the issue by explicitly setting
owneron contextualized credentials. This also needs jenkinsci/github-branch-source-plugin#803 to make it work. I manually tested this PR withgithub-branch-source-pluginsnapshot. The owner is inferred as expected and the build runs without any exception.