Add --label to Build/Publish Docker Image-Step #769

glance- · 2020-01-20T12:33:07Z

This adds the ability to add labels to a docker image build.

This is usefull for example encoding build number, git hash and so on into the docker image.

pjdarton

I've added numerous review comments where there's stuff that's changed which (I think) needs modifying.
gitHub doesn't let me comment on places where there's no changes but needs changes so I'll mention those here...

If we're adding data to DockerBuildImageAction then we should also display that data (otherwise there's little point in storing it).
There's two jelly files, src/main/resources/.../DockerBuildImageAction/index.jelly and summary.jelly which are nigh-on identical and show the containerHost, containerId and tags fields from the DockerBuildImageAction instance. If we're going to store the tags then we should handle those in the jelly so that they get displayed too.
FYI there's an example of good jelly syntax for this in the openstack-cloud plugin's JCloudsComputer/main.jelly file where it lists the liveOpenstackServerDetails and openstackSlaveData maps.
Note: any such code MUST handle null label maps; if a user uses this enhanced code to look at a build built prior to these changes, they won't have a labels field persisted so it'll be de-serialised as null. The jelly code must not NPE under such circumstances.

DockerBuilderPublisher.DescriptorImpl needs to have a doCheck... method added for the new field that the UI is now handling (currently labels but I think it should be labelsString).
This needs to return a FormValidation indicating what (if anything) is wrong with what the user specified. See the doCheckTagsString method for an example.

pjdarton · 2020-01-20T13:24:39Z

src/main/java/com/nirima/jenkins/plugins/docker/action/DockerBuildImageAction.java

@@ -32,6 +33,7 @@
    public final boolean pushOnSuccess;
    public /* almost final */ boolean noCache;
    public /* almost final */ boolean pull;
+    public Map<String, String> labels;


This should include the /* almost final */ comment - it's only non-final because it can be set by setters; like the other "almost final" fields, it shouldn't change after it's been set.

pjdarton · 2020-01-20T13:27:36Z

src/main/java/com/nirima/jenkins/plugins/docker/action/DockerBuildImageAction.java

+        this(containerHost, containerId, tags, cleanupWithJenkinsJobDelete, pushOnSuccess, noCache, pull);
+        setLabels(labels);
+    }
+


We don't need both these internal-use-only constructors - we should only have one of them.
Combine them so we've only got one "populates all fields" constructor that for internal-use-only ... and ensure that anywhere (in this plugin) that calls the old one is amended to provide the extra parameter.

pjdarton · 2020-01-20T13:29:03Z

src/main/java/com/nirima/jenkins/plugins/docker/builder/DockerBuilderPublisher.java

@@ -133,6 +135,7 @@
    public final String cloud;
    public /* almost final */ boolean noCache;
    public /* almost final */ boolean pull;
+    public Map<String, String> labels;


This should have the @CheckForNull annotation because it can be null.
...and everywhere that uses it should be null-proof.

See comment below - I think it'd be better to store labelString rather than the parsed result of the string the user specified.
If we only store the parsed result, we can't store an invalid result, meaning that anything the user types in that we can't parse gets destroyed rather than letting the user go back and re-edit it.

..and, also, it should be /* almost final */ too, because it's only set by the setter at construction time - it's not really something that's mutated while the code is running.

pjdarton · 2020-01-20T13:38:05Z

src/main/java/com/nirima/jenkins/plugins/docker/builder/DockerBuilderPublisher.java

@@ -207,6 +210,34 @@ public void setPull(boolean pull) {
        this.pull = pull;
    }

+    public String getLabels() {


The established convention within this plugin (e.g. the "tags" field) is that where we've got a collection that's also available in string form (for the UI), we have getX and @DataBoundSetter setX methods that return/take the raw type and getXString and setXString methods that return/take Strings.

So a method called getLabels should return Map<String, String> and the method defined here should be called getLabelsString.
Similarly, just below, the setLabels method should be the one annotated with @DataBoundSetter.

...and both the .jelly code and the doCheck... methods in the Descriptior (for the UI) will then need to refer to the labelsString field.

I was thinking about this this morning... I have a better idea.

We should store the labelsString field. There should be a getter and setter for this field.

We should also have a private static Map<String, String> parseLabels(String labelsString) method which can throw IllegalArgumentException if it's given an invalid string (and returns null if there's no labels), and a public Map<String, String> getLabels() method which calls it with the labelsString field value.

This way, the code will keep and persist whatever the user enters into the WebUI, we can use the parseLabels method in the doCheckLabelsString method in the Descriptor class to provide users will fast feedback about the validity of what they've typed in, and we'll get the same (consistent) exception messages in the error logs if the user enters in borked data and then saves that.

pjdarton · 2020-01-20T13:42:00Z

src/main/java/com/nirima/jenkins/plugins/docker/builder/DockerBuilderPublisher.java

+    @DataBoundSetter
+    public void setLabels(String labelsString) {
+        setLabels(isEmpty(labelsString) ?
+                Collections.EMPTY_MAP :


It'd be better to store "no labels" as a null Map instead of an empty one.
This way, the code knows that the collection is either null or it's got some content, and it also means that the serialized form of this class has no labels element when there's no labels, which is more efficient and exactly the same as we'd get if we had builds defined before this feature was added.

This decision also affects code later where we (currently) call .withLabels(labels) unconditionally; we are (unintentionally) relying on the docker engine to treat "labels not specified in request" exactly the same as "labels were specified to be empty".

pjdarton · 2020-01-20T13:49:18Z

...main/resources/com/nirima/jenkins/plugins/docker/builder/DockerBuilderPublisher/config.jelly

@@ -47,4 +47,8 @@
        <f:checkbox default="false"/>
    </f:entry>

+    <f:entry title="${%Label list}" field="labels">


If we follow the getter/setter convention as used by the tags field, this will have to change to field="labelsString".
See earlier comments for full explanation.

pjdarton · 2020-01-20T13:51:41Z

.../resources/com/nirima/jenkins/plugins/docker/builder/DockerBuilderPublisher/help-labels.html

@@ -0,0 +1,4 @@
+<div>
+    If set, builds the image with <code>--labels</code> to add labels to the image built


Need a full stop at the end of this line.

pjdarton · 2020-01-20T13:53:21Z

.../resources/com/nirima/jenkins/plugins/docker/builder/DockerBuilderPublisher/help-labels.html

@@ -0,0 +1,4 @@
+<div>
+    If set, builds the image with <code>--labels</code> to add labels to the image built
+    See the docker <a href="https://docs.docker.com/engine/reference/commandline/build/">build command</a> for more information.


Best to ensure that this text starts on a newline when rendered.
e.g. put a line with just   before this line.

pjdarton · 2020-01-20T14:02:33Z

.../resources/com/nirima/jenkins/plugins/docker/builder/DockerBuilderPublisher/help-labels.html

@@ -0,0 +1,4 @@
+<div>


If we follow the getter/setter convention as used by the tags field, this file will have to be renamed to help-labelsString.html.
See earlier comments for full explanation.

pjdarton · 2020-01-20T14:05:28Z

.../resources/com/nirima/jenkins/plugins/docker/builder/DockerBuilderPublisher/help-labels.html

+<div>
+    If set, builds the image with <code>--labels</code> to add labels to the image built
+    See the docker <a href="https://docs.docker.com/engine/reference/commandline/build/">build command</a> for more information.
+</div>


General note about this:
I've just checked the documentation regarding labels on the docker website and I'm sorry to say that it's woefully incomplete - there's no mention of the syntax at all 😞
So we can't rely on that site to tell users about this - we're going to have to explain to the user what the syntax is.
...and if we do that, it'd probably be best to have 3 sections to this content - the initial summary line, then a  ...  paragraph explaining the syntax required by this plugin (i.e. LabelName=LabelValue, one per line), and then the final "See ... for more information" line (which then wouldn't need a   before it if it had a  before it).

We don't need a massive essay, but we do need to tell folks it's multiple entries (one per line) and what each line's syntax is. Take a look at the tagsString.html file as an example.

pjdarton · 2020-01-20T14:56:07Z

Thanks for the PR - it's a very welcome enhancement.
I've reviewed the code - some modifications needed, but nothing too horrible (I hope).

glance- · 2020-01-31T08:56:21Z

I'm sorry to inform that I'm going offline for a while transitioning to a new job. This work was done as a side-task for my previous job, and I'm not sure if I'm going to get around to picking this up and finishing it. Maybe one of my old colleagues will pick this up and finish it.

That said, anyone is welcome to finish, steal, rework or just get inspired by this and do what they feel for with it.

pjdarton · 2020-02-03T16:43:44Z

OK, thanks for letting me know; I'll know not to keep poking you for progress 😉
I'll leave this "open" for a while (to help folks find it) but will (eventually) close it if there's no further progress.

To anyone else finding this issue in the future: If you fancy continuing where this one left off, that'd be appreciated.

pjdarton · 2022-06-09T11:05:56Z

As there's been no movement on this for over a year, I'm going to close it.
If anyone still wants this, feel free to re-open this (or a new PR), resolve the conflicts and requested changes and I can take another look at merging it.

Add --label to Build/Publish Docker Image-Step

c94c33e

pjdarton suggested changes Jan 20, 2020

View reviewed changes

glance- mentioned this pull request Jan 21, 2020

Encode build information into docker images as labels SUNET/bootstrap-docker-builds#10

Open

pjdarton added the WIP PR that is *not* ready for merging (but hopefully being worked on by the author). label Feb 3, 2020

pjdarton added the enhancement A PR providing an enhancement to existing functionality. label Mar 11, 2020

pjdarton closed this Jun 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --label to Build/Publish Docker Image-Step #769

Add --label to Build/Publish Docker Image-Step #769

glance- commented Jan 20, 2020

pjdarton left a comment

pjdarton Jan 20, 2020

pjdarton Jan 20, 2020

pjdarton Jan 20, 2020

pjdarton Jan 21, 2020

pjdarton Jan 21, 2020

pjdarton Jan 20, 2020

pjdarton Jan 21, 2020

pjdarton Jan 20, 2020

pjdarton Jan 20, 2020

pjdarton Jan 20, 2020

pjdarton Jan 20, 2020

pjdarton Jan 20, 2020

pjdarton Jan 20, 2020

pjdarton commented Jan 20, 2020

glance- commented Jan 31, 2020

pjdarton commented Feb 3, 2020

pjdarton commented Jun 9, 2022

		@@ -0,0 +1,4 @@
		<div>
		If set, builds the image with <code>--labels</code> to add labels to the image built

Add --label to Build/Publish Docker Image-Step #769

Add --label to Build/Publish Docker Image-Step #769

Conversation

glance- commented Jan 20, 2020

pjdarton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjdarton commented Jan 20, 2020

glance- commented Jan 31, 2020

pjdarton commented Feb 3, 2020

pjdarton commented Jun 9, 2022