Skip to content

[JENKINS-51395] Syntax problems in withDockerRegistry #138

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.

Sign up for GitHub

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

Merged
jglick merged 7 commits into from
May 17, 2018
Merged

[JENKINS-51395] Syntax problems in withDockerRegistry #138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
732e1bb
[JENKINS-51395] At least reproduced problem in test.
jglick May 17, 2018
8c463ba
[JENKINS-51395] Switching recommended syntax to inline arguments, whi…
jglick May 17, 2018
ce736d3
Also noticed that snippet generation was inappropriate for using the …
jglick May 17, 2018
c9ae684
Whitespace changes to minimize diff.
jglick May 17, 2018
15c0acc
Noting that usability of ServerEndpointStep could benefit from a simi…
jglick May 17, 2018
ebdfe94
@svanoort points out that it is safer to copy the arguments map befor…
jglick May 17, 2018
a03096b
Merge branch 'master' into withDockerRegistry-syntax-JENKINS-51395
jglick May 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
<java.level>8</java.level>
<workflow-step-api-plugin.version>2.9</workflow-step-api-plugin.version>
<workflow-support-plugin.version>2.12</workflow-support-plugin.version>
<workflow-cps-plugin.version>2.25</workflow-cps-plugin.version>
</properties>
<repositories>
<repository>
Expand All @@ -57,7 +58,7 @@
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-cps</artifactId>
<version>2.25</version>
<version>${workflow-cps-plugin.version}</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
Expand Down Expand Up @@ -103,6 +104,13 @@
<classifier>tests</classifier>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-cps</artifactId>
<version>${workflow-cps-plugin.version}</version>
<classifier>tests</classifier>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-job</artifactId>
Expand Down
32 changes: 31 additions & 1 deletion src/main/java/org/jenkinsci/plugins/docker/workflow/RegistryEndpointStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,24 @@
import hudson.Extension;
import hudson.FilePath;
import hudson.Launcher;
import hudson.Util;
import hudson.model.Job;
import hudson.model.Node;
import hudson.model.TaskListener;
import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.TreeMap;
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import org.jenkinsci.plugins.docker.commons.credentials.DockerRegistryEndpoint;
import org.jenkinsci.plugins.docker.commons.credentials.KeyMaterialFactory;
import org.jenkinsci.plugins.docker.commons.tools.DockerTool;
import org.jenkinsci.plugins.structs.describable.UninstantiatedDescribable;
import org.jenkinsci.plugins.workflow.steps.AbstractStepDescriptorImpl;
import org.jenkinsci.plugins.workflow.steps.AbstractStepImpl;
import org.jenkinsci.plugins.workflow.steps.Step;
import org.jenkinsci.plugins.workflow.steps.StepContextParameter;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.DataBoundSetter;
Expand All @@ -62,7 +69,7 @@ public String getToolName() {
}

@DataBoundSetter public void setToolName(String toolName) {
this.toolName = toolName;
this.toolName = Util.fixEmpty(toolName);
}

public static class Execution extends AbstractEndpointStepExecution {
Expand Down Expand Up @@ -105,6 +112,29 @@ public DescriptorImpl() {
return true;
}

@Override public UninstantiatedDescribable uninstantiate(Step step) throws UnsupportedOperationException {
RegistryEndpointStep s = (RegistryEndpointStep) step;
Map<String, Object> args = new TreeMap<>();
args.put("url", s.registry.getUrl());
args.put("credentialsId", s.registry.getCredentialsId());
args.put("toolName", s.toolName);
args.values().removeAll(Collections.singleton(null));
return new UninstantiatedDescribable(args);
}

@Override public Step newInstance(Map<String, Object> arguments) throws Exception {
arguments = new HashMap<>(arguments);
if (arguments.containsKey("url") || arguments.containsKey("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")));
Copy link
Member

@svanoort svanoort May 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@svanoort svanoort May 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, the test covers this.

Copy link
Member Author

@jglick jglick May 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: '…') {…}

} else if (!arguments.containsKey("registry")) {
throw new IllegalArgumentException("must specify url/credentialsId (or registry)");
}
return super.newInstance(arguments);
}

}

}
2 changes: 2 additions & 0 deletions src/main/java/org/jenkinsci/plugins/docker/workflow/ServerEndpointStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ public DescriptorImpl() {
return true;
}

// TODO allow DockerServerEndpoint fields to be inlined, as in RegistryEndpointStep, so Docker.groovy can say simply: script.withDockerServer(uri: uri, credentialsId: credentialsId) {…}

}

}
2 changes: 1 addition & 1 deletion src/main/resources/org/jenkinsci/plugins/docker/workflow/Docker.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class Docker implements Serializable {
public <V> V withRegistry(String url, String credentialsId = null, Closure<V> body) {
node {
script.withEnv(["DOCKER_REGISTRY_URL=${url}"]) {
script.withDockerRegistry(registry: [url: url, credentialsId: credentialsId], toolName: script.env.DOCKER_TOOL_NAME) {
script.withDockerRegistry(url: url, credentialsId: credentialsId, toolName: script.env.DOCKER_TOOL_NAME) {
body()
}
}
Expand Down
72 changes: 43 additions & 29 deletions src/test/java/org/jenkinsci/plugins/docker/workflow/RegistryEndpointStepTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,45 +28,59 @@
import com.cloudbees.plugins.credentials.common.IdCredentials;
import com.cloudbees.plugins.credentials.domains.Domain;
import com.cloudbees.plugins.credentials.impl.UsernamePasswordCredentialsImpl;
import java.util.Collections;
import java.util.Map;
import java.util.TreeMap;
import org.jenkinsci.plugins.docker.commons.credentials.DockerRegistryEndpoint;
import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
import org.jenkinsci.plugins.workflow.cps.SnippetizerTester;
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
import org.jenkinsci.plugins.workflow.steps.StepConfigTester;
import org.jenkinsci.plugins.workflow.structs.DescribableHelper;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import org.junit.ClassRule;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runners.model.Statement;
import org.jvnet.hudson.test.BuildWatcher;
import org.jvnet.hudson.test.RestartableJenkinsRule;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;

public class RegistryEndpointStepTest {

@ClassRule public static BuildWatcher buildWatcher = new BuildWatcher();
@Rule public RestartableJenkinsRule story = new RestartableJenkinsRule();
@Rule public JenkinsRule r = new JenkinsRule();

@Test public void configRoundTrip() {
story.addStep(new Statement() {
@Override public void evaluate() throws Throwable {
IdCredentials registryCredentials = new UsernamePasswordCredentialsImpl(CredentialsScope.GLOBAL, "registryCreds", null, "me", "pass");
CredentialsProvider.lookupStores(story.j.jenkins).iterator().next().addCredentials(Domain.global(), registryCredentials);
StepConfigTester sct = new StepConfigTester(story.j);
Map<String,Object> registryConfig = new TreeMap<String,Object>();
registryConfig.put("url", "https://docker.my.com/");
registryConfig.put("credentialsId", registryCredentials.getId());
Map<String,Object> config = Collections.<String,Object>singletonMap("registry", registryConfig);
RegistryEndpointStep step = DescribableHelper.instantiate(RegistryEndpointStep.class, config);
step = sct.configRoundTrip(step);
DockerRegistryEndpoint registry = step.getRegistry();
assertNotNull(registry);
assertEquals("https://docker.my.com/", registry.getUrl());
assertEquals(registryCredentials.getId(), registry.getCredentialsId());
assertEquals(config, DescribableHelper.uninstantiate(step));
}
});
@Issue("JENKINS-51395")
@Test public void configRoundTrip() throws Exception {
{ // Recommended syntax.
SnippetizerTester st = new SnippetizerTester(r);
RegistryEndpointStep step = new RegistryEndpointStep(new DockerRegistryEndpoint("https://myreg/", null));
step.setToolName("");
st.assertRoundTrip(step, "withDockerRegistry(url: 'https://myreg/') {\n // some block\n}");
step = new RegistryEndpointStep(new DockerRegistryEndpoint(null, "hubcreds"));
st.assertRoundTrip(step, "withDockerRegistry(credentialsId: 'hubcreds') {\n // some block\n}");
step = new RegistryEndpointStep(new DockerRegistryEndpoint("https://myreg/", "mycreds"));
step.setToolName("ce");
st.assertRoundTrip(step, "withDockerRegistry(credentialsId: 'mycreds', toolName: 'ce', url: 'https://myreg/') {\n // some block\n}");
}
{ // Older syntax.
WorkflowJob p = r.createProject(WorkflowJob.class, "p");
p.setDefinition(new CpsFlowDefinition("node {withDockerRegistry(registry: [url: 'https://docker.my.com/'], toolName: 'irrelevant') {}}", true));
r.buildAndAssertSuccess(p);
p.setDefinition(new CpsFlowDefinition("node {withDockerRegistry(registry: [url: 'https://docker.my.com/']) {}}", true));
r.buildAndAssertSuccess(p);
p.setDefinition(new CpsFlowDefinition("node {withDockerRegistry([url: 'https://docker.my.com/']) {}}", true));
r.buildAndAssertSuccess(p);
// and new, just in case SnippetizerTester is faking it:
p.setDefinition(new CpsFlowDefinition("node {withDockerRegistry(url: 'https://docker.my.com/') {}}", true));
r.buildAndAssertSuccess(p);
}
{ // UI form.
IdCredentials registryCredentials = new UsernamePasswordCredentialsImpl(CredentialsScope.GLOBAL, "registryCreds", null, "me", "pass");
CredentialsProvider.lookupStores(r.jenkins).iterator().next().addCredentials(Domain.global(), registryCredentials);
StepConfigTester sct = new StepConfigTester(r);
RegistryEndpointStep step = new RegistryEndpointStep(new DockerRegistryEndpoint("https://docker.my.com/", "registryCreds"));
step = sct.configRoundTrip(step);
DockerRegistryEndpoint registry = step.getRegistry();
assertNotNull(registry);
assertEquals("https://docker.my.com/", registry.getUrl());
assertEquals("registryCreds", registry.getCredentialsId());
// TODO check toolName
}
}

}