Use CredentialsProvider.findCredentialById whenever possible
#1242
Conversation
|
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.
| @@ -829,13 +828,13 @@ private PollingResult compareRemoteRevisionWithImpl(Job<?, ?> project, Launcher | |||
| * @throws InterruptedException when interrupted | |||
| */ | |||
| @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. |
Required for jenkinsci/github-branch-source-plugin#527 to take advantage of jenkinsci/credentials-plugin#293. Otherwise
withCredentialsworks butcheckout scmdoes not.