Use CredentialsProvider.findCredentialById whenever possible#1242
Use CredentialsProvider.findCredentialById whenever possible#1242MarkEWaite merged 3 commits intojenkinsci:masterfrom
CredentialsProvider.findCredentialById whenever possible#1242Conversation
MarkEWaite
left a comment
MarkEWaite
left a comment
There was a problem hiding this comment.
Two questions that do not block the merge. The change looks good to me. I'm running this build along with the credentials plugin and the GitHub branch source plugin that match it. No issues detected. I haven't added an app credential to test the new path.
|
I added some more explanation of the API and its usage from this PR in jenkinsci/credentials-plugin#293. |
|
@jglick I think this is ready to merge and be released. Any reason that I should not merge it and release it so that it is available in a released version? |
…nt` overloads. This allows methods to be simplified by dropping the `Job` / `Item` parameter; and allows us to drop the `CredentialsProvider.lookupCredentials` code branch, so that `CredentialsProvider.findCredentialById` can be called whenever there are credentials.
| */ | ||
| @NonNull | ||
| public GitClient createClient(TaskListener listener, EnvVars environment, Run<?,?> build, FilePath workspace) throws IOException, InterruptedException { | ||
| public GitClient createClient(TaskListener listener, EnvVars environment, @NonNull Run<?,?> build, FilePath workspace) throws IOException, InterruptedException { |
There was a problem hiding this comment.
I started off inspecting https://cs.github.com/?scope=org%3Ajenkinsci&scopeName=jenkinsci&q=GitSCM.createClient to be sure adding @NonNull here was safe, until I realized that we are dereferencing it just below anyway!
| if (project != null && project.getLastBuild() != null) { | ||
| CredentialsProvider.track(project.getLastBuild(), credentials); | ||
| } | ||
| CredentialsProvider.track(build, credentials); // TODO unclear if findCredentialById was meant to do this in all cases |
There was a problem hiding this comment.
See comment in upstream PR. Seems to have been forgotten. Probably cleaner to have track be called upstream, but deferring that for now.
| project instanceof Queue.Task | ||
| ? ((Queue.Task) project).getDefaultAuthentication() | ||
| : ACL.SYSTEM, |
There was a problem hiding this comment.
findCredentialById has a rather subtler sequence of logic which I do not entirely follow, but seems best to use the official version. (Apparently relevant to supporting credentials parameters, input step, etc.)
If I thought there were, I would mark it as draft! But thanks for the tip about |
|
I've reviewed the most recent changes and they look very good to me. I'll merge, run some additional interactive testing, and then release it. |
3 participants
Required for jenkinsci/github-branch-source-plugin#527 to take advantage of jenkinsci/credentials-plugin#293. Otherwise
withCredentialsworks butcheckout scmdoes not.