[JENKINS-57439 ] - Add declarative support #77

lion7 · 2019-04-30T20:38:17Z

This PR implements the credentials handler for a declarative pipeline, similiar to jenkinsci/aws-credentials-plugin#50 and inspired by FileCredentialsHandler.java
The dependency on pipeline-model-extensions is made optional as suggested by @jglick in jenkinsci/aws-credentials-plugin#50,

Additionally I also added a symbol annotation to the DockerTool descriptor so it can be used in the tools section of a declarative pipeline (inspired by GradleInstallation.java)

This allows to use the Docker tool and Docker credentials like this:

environment {
  DOCKER_CERT_PATH = credentials('id-for-a-docker-cred')
}
tools {
  docker '18.09'
}
stages {
  stage('foo') {
    steps {
      sh "docker version" // DOCKER_CERT_PATH is automatically picked up by the Docker client
    }
  }
}

Note that I had to bump a lot of dependency versions due to the enforcer plugin. I hope this is ok.

…eline

lion7 · 2019-05-02T17:48:28Z

What's the usual proces for getting a PR like this reviewed? I see that most PRs are usually reviewed by @jglick, @jvz or @dwnusbaum?

dwnusbaum

Thanks for the PR! Dependency updates are no problem. Would be good for @abayer to take a look to make sure the Declarative extension looks ok. Requesting changes just because you need @Extension(optional = true) to be able to mark the dependency as optional.

pom.xml

dwnusbaum · 2019-05-02T18:43:27Z

pom.xml

      <scope>test</scope>
    </dependency>
    <dependency>
      <groupId>org.jenkins-ci.plugins.workflow</groupId>
      <artifactId>workflow-cps</artifactId>
-      <version>2.39</version>
+      <version>2.67</version>


nit: It would be good to introduce properties for the versions of workflow-step-api, workflow-cps, and workflow-support, since they are each used twice and we don't want them to get out of sync.

Good suggestion, I created a few properties for those deps

Thanks! Minor nits but workflow-step-cps and workflow-step-support should just be workflow-cps and workflow-support (workflow-step-api is correct), so it would be great to go ahead and fix that.

...in/java/org/jenkinsci/plugins/docker/commons/credentials/DockerServerCredentialsHandler.java

lion7 · 2019-05-02T19:04:18Z

Thanks for your quick review @dwnusbaum 😄

lion7 · 2019-05-02T19:27:16Z

Whoops, committing without running a local build is a bad idea 🙈

Anyway, the lowest compatible Jenkins version seems to be 2.150.1.

lion7 · 2019-05-02T19:35:16Z

By the way, the build fails with a No space left on device...

abayer · 2019-05-02T19:53:00Z

I've kicked off a new build which should get a new agent.

abayer

Looks reasonable to me.

lion7 · 2019-05-03T06:44:43Z

I've kicked off a new build which should get a new agent.

The build still fails with the same error 😢

abayer · 2019-05-03T10:56:56Z

There was one agent that just didn’t want to go away despite being out of disk space - I’ve nuked it by hand and rerun.

lion7 · 2019-05-03T16:55:23Z

My next (probably obvious) question: how are things normally merged? Does a contributor merge this PR? And how/when are changes like this usually released and made available in the Jenkins plugin portal?
Currently I installed a locally build version of this plugin in our corporate Jenkins but the IT support team isn't really happy with customized plugins (both from a maintenance and security perspective) seeing that they manage a dozen instances of Jenkins.

jglick

There is no test here, though DockerServerCredentialsBindingTest covers most of the logic.

Is this feature really necessary? You can already use withDockerServer in steps, which has the advantage of better localizing the use of the credentials to a particular block.

jglick · 2019-05-03T20:02:06Z

src/main/java/org/jenkinsci/plugins/docker/commons/tools/DockerTool.java

@@ -114,7 +115,8 @@ public DockerTool forNode(Node node, TaskListener log) throws IOException, Inter
        }
    }

-    @Extension public static class DescriptorImpl extends ToolDescriptor<DockerTool> {
+    @Extension @Symbol("docker")


This should be a separate PR. See #52.

Agreed, especially after reading the discussion in #52 . It looked like a simple change so I just included it in this PR. But I will move it to a separate PR.

lion7 · 2019-05-04T12:38:24Z

I'll try to add a test that tests the new class explicitly instead of relying on DockerServerCredentialsBindingTest.

As for whether this change is really necessary: it depends I geuss. In our workflow we try to stick as much as possible to declarative pipelines to keep things simple and understandable (writing this comment reminds me of https://xkcd.com/1172/).

Its true that we could use withDockerServer (actually docker.withServer() since the step itself shouldn't be used directly) but that means I have to repeat this in every stage and also I'd probably have to use a script block since docker.with***() refers to a global variable and not a step. IMO that kind of defeats the whole purpose of using a declarative pipeline, in that case I might as well use a scripted pipeline instead.

What I'm trying to archieve is running a multi-stage build on a Docker EE UCP cluster. Most of our build tool logic (we're using Gradle) is inside the Dockerfile, so basicly Jenkins only has to run a docker build, passing a few credentials via build arguments and then push the resulting image to our corporate Docker registry. I'll try to think of a few more alternative next week to figure out if I can archieve a working pipeline without bringing in too much complexity in our pipeline code.

jglick · 2019-05-04T13:57:47Z

The docker global variable is incompatible with Declarative and IMHO should never have been created to begin with. withDockerServer is fine and should work in Declarative. But yes it would need to be used separately in each stage requiring access to the Docker daemon. (Though I thought I remembered some provision in Declarative by which a block-scoped step could be used automatically in an options block?)

To be clear, I am not trying to block this PR, just clarify the use case vs. alternatives.

…tive pipeline" This reverts commit c5b21d7

lion7 · 2019-05-04T14:51:46Z

Hmm, the only steps that seem to be usable in an options block are the retry and timestamps steps. I'm not entirely sure how that works, but if the withDockerServer step could be used in an options block then this PR isn't that necessary anymore since that would enable me to define the Docker daemon to use for the entire pipeline or (set of) stage(s). I geuss the same goes for the withDockerRegistry step.

No offense taken in your earlier comment btw, I totally agree that it's better to discuss alternatives before blindly merging changes.

On a sidenote, if the docker global variable is a bad idea then maybe it should be promoted less in the documentation? Currently its use is encouraged and the use of the individual steps are discouraged.

lion7 · 2019-05-06T09:04:49Z

I added a test for the new functionality. I did have to add some dependencies to allow the usage of a declarative pipeline in the test code.

As a practical alternative I attached our UCP manager node as an agent to Jenkins. This way both the docker command and Docker certificates are available locally on the agent, so in my pipeline I only have to specify the label for this specific agent. The downside of this is that a part of the pipeline configuration is now 'hidden' in the node configuration. We have yet to discuss internally if that is acceptable or that we'd rather be explicit about this configuration in the pipeline code.

lion7 · 2019-05-12T13:46:35Z

Are there any plans to merge this soon? Just hoping this PR won't die a silent death 😛

oleg-nenashev · 2019-05-13T08:04:43Z

resolved the conflict. Will review ASAP and merge if everything is fine from my PoV

oleg-nenashev

I think it is good to go. Thanks @lion7 !

lion7 · 2019-05-13T08:24:16Z

Thanks @oleg-nenashev 😄

oleg-nenashev · 2019-05-13T08:28:14Z

@lion7 is there a Jenkins JIRA ticket for it so that I reference it in the changelog?

oleg-nenashev · 2019-05-13T10:01:08Z

@lion7 my plan is to cut the release today unless there is a negative feedback.
Would it make sense to add a Pipeline support section to the README.md and to add the sample from the PR description there? Some documentation in the plugin would not hurt for sure

lion7 · 2019-05-13T13:25:33Z

@oleg-nenashev I added the example in this PR to the README.
There is no JIRA issue yet, and I can't create a JIRA account because the email containing the password won't show up in my inbox 😞. I tried with both a GMail and Outlook.com account...

oleg-nenashev · 2019-05-13T21:09:19Z

Created https://issues.jenkins-ci.org/browse/JENKINS-57439 . Not sure what causes the JIRA access issue tho, maybe it makes sense to contact the Jenkins INFRA team

lion7 added 3 commits April 30, 2019 22:14

Reincrementalified.

91c9113

Add declarative credentials handler

4cbc8c8

Add symbol for docker tool to simplify its usage in a declarative pip…

c5b21d7

…eline

dwnusbaum requested changes May 2, 2019

View reviewed changes

Changes due to review comments

429830a

Changes due to review comments

dbb1ce6

abayer approved these changes May 2, 2019

View reviewed changes

dwnusbaum approved these changes May 3, 2019

View reviewed changes

jglick reviewed May 3, 2019

View reviewed changes

Revert "Add symbol for docker tool to simplify its usage in a declara…

36742ac

…tive pipeline" This reverts commit c5b21d7

Added a test for the DockerServerCredentialsHandler class

9fd81f6

Merge branch 'master' into master

0b1df3e

Update pom.xml

f40b798

oleg-nenashev approved these changes May 13, 2019

View reviewed changes

Update README.md

6db0d2d

dwnusbaum approved these changes May 13, 2019

View reviewed changes

oleg-nenashev changed the title ~~Add declarative support~~ [JENKINS-57439 ] - Add declarative support May 13, 2019

oleg-nenashev merged commit c3dcc3a into jenkinsci:master May 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-57439 ] - Add declarative support #77

[JENKINS-57439 ] - Add declarative support #77

lion7 commented Apr 30, 2019 •

edited

Loading

lion7 commented May 2, 2019

dwnusbaum left a comment

dwnusbaum May 2, 2019

lion7 May 2, 2019

dwnusbaum May 2, 2019 •

edited

Loading

lion7 commented May 2, 2019

lion7 commented May 2, 2019

lion7 commented May 2, 2019

abayer commented May 2, 2019

abayer left a comment

lion7 commented May 3, 2019

abayer commented May 3, 2019

lion7 commented May 3, 2019

jglick left a comment

jglick May 3, 2019

lion7 May 4, 2019

lion7 commented May 4, 2019

jglick commented May 4, 2019

lion7 commented May 4, 2019

lion7 commented May 6, 2019

lion7 commented May 12, 2019

oleg-nenashev commented May 13, 2019

oleg-nenashev left a comment

lion7 commented May 13, 2019

oleg-nenashev commented May 13, 2019

oleg-nenashev commented May 13, 2019 •

edited

Loading

lion7 commented May 13, 2019 •

edited

Loading

oleg-nenashev commented May 13, 2019

[JENKINS-57439 ] - Add declarative support #77

[JENKINS-57439 ] - Add declarative support #77

Conversation

lion7 commented Apr 30, 2019 • edited Loading

lion7 commented May 2, 2019

dwnusbaum left a comment

Choose a reason for hiding this comment

dwnusbaum May 2, 2019

Choose a reason for hiding this comment

lion7 May 2, 2019

Choose a reason for hiding this comment

dwnusbaum May 2, 2019 • edited Loading

Choose a reason for hiding this comment

lion7 commented May 2, 2019

lion7 commented May 2, 2019

lion7 commented May 2, 2019

abayer commented May 2, 2019

abayer left a comment

Choose a reason for hiding this comment

lion7 commented May 3, 2019

abayer commented May 3, 2019

lion7 commented May 3, 2019

jglick left a comment

Choose a reason for hiding this comment

jglick May 3, 2019

Choose a reason for hiding this comment

lion7 May 4, 2019

Choose a reason for hiding this comment

lion7 commented May 4, 2019

jglick commented May 4, 2019

lion7 commented May 4, 2019

lion7 commented May 6, 2019

lion7 commented May 12, 2019

oleg-nenashev commented May 13, 2019

oleg-nenashev left a comment

Choose a reason for hiding this comment

lion7 commented May 13, 2019

oleg-nenashev commented May 13, 2019

oleg-nenashev commented May 13, 2019 • edited Loading

lion7 commented May 13, 2019 • edited Loading

oleg-nenashev commented May 13, 2019

lion7 commented Apr 30, 2019 •

edited

Loading

dwnusbaum May 2, 2019 •

edited

Loading

oleg-nenashev commented May 13, 2019 •

edited

Loading

lion7 commented May 13, 2019 •

edited

Loading