[JENKINS-48050] introduce "container" agent type by ndeloof · Pull Request #255 · jenkinsci/pipeline-model-definition-plugin

ndeloof · 2018-03-13T18:28:54Z

JENKINS issue(s):
https://issues.jenkins-ci.org/browse/JENKINS-48050
Description:
introduce "container" agent type with plugable implementation, + default docker-plugin based implementation
Users/aliases to notify:
@abayer

…ne.option

introduce ContainerAgentProvisioner extension point default implementation based on docker-plugin Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

abayer · 2018-03-14T11:45:02Z

Needs a time stamped SNAPSHOT of docker-plugin deployed and used as the dependency.

abayer

I know this is work in progress, but it really, really needs tests.

abayer · 2018-03-14T11:46:24Z

        }
    }
+
+    public static ContainerAgentProvider getProvider() {


Could we get some javadoc here and elsewhere to explain what these are doing?

abayer · 2018-03-14T11:47:01Z

+/**
+ * @author <a href="mailto:nicolas.deloof@gmail.com">Nicolas De Loof</a>
+ */
+public class ContainerAgentProvider {


Definitely needs javadoc since there’s nothing actually in here to help figure it out.

Ah, I see this is incomplete - this looks, from other code, to actually be an extension point?

would "Provides ContainerAgent" javadoc be really helpful here ?
isn't class name obvious ?

abayer · 2018-03-14T11:48:06Z

+    <dependency>
+      <groupId>io.jenkins.docker</groupId>
+      <artifactId>docker-plugin</artifactId>
+      <version>1.2-SNAPSHOT</version>


Use the time stamped snapshot version here.

correct me if I'm wrong, but we don't publish timestamped snapshots for plugins' pull-request builds
https://ci.jenkins.io/job/Plugins/job/docker-plugin/job/JENKINS-48050/8/console

Correct. You need to do a mvn deploy by hand. Also, make sure these new dependencies are in dependencyManagement in the parent pom.

abayer · 2018-03-14T11:50:33Z

+  -->
+
+<p>
+    The job or stage will run within a Docker container.


Please add a config.jelly for supporting the changes coming in #250

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

ndeloof · 2018-03-14T15:18:04Z

I know this is work in progress, but it really, really needs tests.

see 6bd5444

abayer · 2018-03-14T17:36:22Z

@ndeloof Thanks - still needs more tests for making sure things like environment interact properly, stage-level agents...it would be great to see if https://issues.jenkins-ci.org/browse/JENKINS-48319 and https://issues.jenkins-ci.org/browse/JENKINS-46831 are issues with container or not, etc.

ndeloof · 2018-03-14T21:59:22Z

Those are already covered by https://github.com/jenkinsci/docker-plugin/blob/master/src/test/java/io/jenkins/docker/pipeline/DockerNodeStepTest.java#L181

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

abayer · 2018-03-14T23:55:15Z

@ndeloof I’d still like to see tests in Declarative itself.

ndeloof · 2018-03-15T08:27:35Z

JENKINS-48319, JENKINS-46831: those are irrelevant with container agents are those are just plain agents, without anything shared between a "top level" and "stage" agent. container probably won't address the use case those issues have been reported for. We could write a test case for them with plain label agent as well, this would not demonstrate anything has been fixed.

abayer · 2018-03-15T09:52:43Z

I’d still like tests demonstrating that the issues are not relevant for the container implementation. It’s always better to overdo it with tests than to miss something.

abayer · 2018-03-15T09:59:24Z

And again, please do a mvn deploy of docker-plugin yourself, and use the timestamp SNAPSHOT version as the dependency here.

abayer · 2018-03-15T15:23:34Z

+    private String registryCredentials;
+
+    @DataBoundConstructor
+    public ContainerAgent(@Nonnull String image) {


We need some more setters/fields - String type for overriding the default container implementation, for certain, and probably some way to specify sidecar containers and a way to specify implementation-specific configuration.

sidecar containers is out of scope for JENKINS-48050 phase 1
(but easy to add later)
I don't understand the need for "type". All implementation should create the exact same container deployment, just using distinct API (i.e docker or kubectl)

abayer · 2018-03-15T15:24:20Z

+    }
+
+    @Extension
+    public static class DescriptorImpl extends Descriptor<ContainerAgentProvider> {


Needs a @Symbol for use in specifying an overriding implementation.

abayer · 2018-03-15T15:24:47Z

        this.registry = registry;
    }

+    public ContainerAgentProvider getProvider() {


We need this same logic in FolderConfig too.

abayer · 2018-03-15T15:25:11Z

Please fix the findbugs error and the failing unit tests. Thanks.

pipeline-model to define abstract "container" agent and docker-plugin or kubernetes-plugin for implementation

bitwiseman · 2019-05-27T21:51:52Z

@ndeloof @abayer @jglick
It's been over a year since last update. Should we close or move this forward?

jglick · 2019-05-28T12:08:45Z

@bitwiseman FWIW my counterproposal is jenkinsci/docker-plugin#681. Anything to put a nail in the coffin of docker-workflow (jenkinsci/docker-workflow-plugin#148 (comment)).

ndeloof · 2019-09-06T14:16:04Z

inactive, feel free to take over

ndeloof added 2 commits March 1, 2018 15:35

[JENKINS-49748] change implementation depending on a top level pipeli…

294144c

…ne.option

introduce "container" agent

b33d625

introduce ContainerAgentProvisioner extension point default implementation based on docker-plugin Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

abayer requested review from abayer and rsandell March 13, 2018 21:32

abayer reviewed Mar 14, 2018

View reviewed changes

ndeloof added 3 commits March 14, 2018 14:00

config.jelly for ContainerAgent

8cb0ec2

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

mark as ExtensionPoint + javadoc

8353a8c

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

a test case

6bd5444

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

assumeDocker

8a1bde9

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

use timestamp SNAPSHOT dependency to demonstrate feature

e625240

abayer reviewed Mar 15, 2018

View reviewed changes

reverse dependency

e2f0789

pipeline-model to define abstract "container" agent and docker-plugin or kubernetes-plugin for implementation

jglick mentioned this pull request Aug 24, 2018

[JENKINS-48050] Declarative Pipeline support for dockerNode jenkinsci/docker-plugin#681

Merged

ndeloof closed this Sep 6, 2019

Conversation

ndeloof commented Mar 13, 2018

Uh oh!

abayer commented Mar 14, 2018

Uh oh!

abayer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ndeloof commented Mar 14, 2018

Uh oh!

abayer commented Mar 14, 2018

Uh oh!

ndeloof commented Mar 14, 2018

Uh oh!

abayer commented Mar 14, 2018

Uh oh!

ndeloof commented Mar 15, 2018

Uh oh!

abayer commented Mar 15, 2018

Uh oh!

abayer commented Mar 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abayer commented Mar 15, 2018

Uh oh!

bitwiseman commented May 27, 2019

Uh oh!

jglick commented May 28, 2019

Uh oh!

ndeloof commented Sep 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants