[JENKINS-51395] Syntax problems in withDockerRegistry#138
[JENKINS-51395] Syntax problems in withDockerRegistry#138jglick merged 7 commits intojenkinsci:masterfrom
Conversation
…le still supporting the old syntax.
…“default” Docker tool.
…lar change, though it is not needed for a bug fix.
| } | ||
|
|
||
| @Override public Step newInstance(Map<String, Object> arguments) throws Exception { | ||
| if (arguments.containsKey("url") || arguments.containsKey("credentialsId")) { |
There was a problem hiding this comment.
Direct mutation of input Map: danger, danger will robinson!
Please create copy if you're going to mutate something (just in case something else is done with the args) -- don't remember off the top of my head but there's a real risk this might break something like ArgumentsActionImpl.
There was a problem hiding this comment.
It's not a hard-blocker for a known-and-significant error, but it's dangerous to be doing this here.
| } | ||
|
|
||
| @Override public Step newInstance(Map<String, Object> arguments) throws Exception { | ||
| if (arguments.containsKey("url") || arguments.containsKey("credentialsId")) { |
There was a problem hiding this comment.
I think you mean url && credentialsId? Or perhaps you need a handling for one but not the other?
There was a problem hiding this comment.
Eh, apparently this is legal. Just weird looking, but legal.
There was a problem hiding this comment.
This is what I meant. You may have a url and/or a credentialsId.
| if (arguments.containsKey("registry")) { | ||
| throw new IllegalArgumentException("cannot mix url/credentialsId with registry"); | ||
| } | ||
| arguments.put("registry", new DockerRegistryEndpoint((String) arguments.remove("url"), (String) arguments.remove("credentialsId"))); |
There was a problem hiding this comment.
Will this actually work if you have a credential but not a URL? Or vice versa?
If not, should print something reasonable as an error.
There was a problem hiding this comment.
Nevermind, the test covers this.
There was a problem hiding this comment.
Yes,
withDockerRegistry(credentialsId: '…') {…}logs you in to Docker Hub rather than a private registry. Conversely, if there were some way to have a non-password-protected private registry (or if it had read-only access and you were not pushing anything), you could
withDockerRegistry(url: '…') {…}
svanoort
left a comment
svanoort
left a comment
There was a problem hiding this comment.
Looks like it has a few small issues to me but is generally reasonable AFAICT
svanoort
left a comment
svanoort
left a comment
There was a problem hiding this comment.
Okay, I think my concerns are addressed after a deeper examination -- the Docker APIs here are... odd.
svanoort
left a comment
svanoort
left a comment
There was a problem hiding this comment.
Much happier without the in-place mutation
3 participants
JENKINS-51395: #133 broke scripts using the notation
which is unfortunately what Pipeline Syntax suggests if you have no
toolName. (@abayer patched bothDSLandSnippetizerTestin jenkinsci/workflow-cps-plugin#49, but the behaviors do not seem to actually match in the case that there is one required structure parameter and an optional parameter.)@oleg-nenashev had written some test coverage for the configuration form of this step long ago, but it did not actually attempt to run the step, and the DSL
docker.withRegistryalways calls the more verbose form.It is no longer possible to compatibly remove the new
toolNameparameter, and anyway we need it, so my current plan is to try to inline theDockerRegistryEndpointparameter, assuming that is even possible.