Namespaces for Container Label Keys #660

eaglerainbow · 2018-06-08T22:02:26Z

As by-product of #658, we have detected that https://docs.docker.com/config/labels-custom-metadata/ requests consumers of docker to use namespaces for their container label keys.

This PR proposes to prefix container label keys used with com.nirima.jenkins.plugins.docker..
Moreover, it refactors the existing coding such that all labels are centrally available at a constant interface.

Mapping between old and new label keys

Old Label Key Identifier	New Label Key Identifier
JenkinsId	com.nirima.jenkins.plugins.docker.JenkinsId
JenkinsServerUrl	com.nirima.jenkins.plugins.docker.JenkinsServerUrl
JenkinsContainerImage	com.nirima.jenkins.plugins.docker.JenkinsContainerImage
JenkinsNodeName	com.nirima.jenkins.plugins.docker.JenkinsNodeName
JenkinsTemplateName	com.nirima.jenkins.plugins.docker.JenkinsTemplateName

eaglerainbow · 2018-06-08T22:02:38Z

Disclaimer: I am currently a little unsure about the lifecycle aspects of this PR: Adding the prefix to the label keys must be (more or less) considered an incompatible change. Though, we are not aware of any consumer of the labels besides ourselves. As such, the (consistent) change of this constant value would not be harming as such. However, with this change applied, we assume that at the point of time the update happens, no containers are left on the docker instance. Otherwise, those docker containers would get "lost".

@pjdarton: What is the committment to our consumers and the administrators w.r.t. updating this plugin? Is there a committment out there that we must be "blue/green deployable"?

pjdarton · 2018-06-09T12:22:30Z

I think that the labels that the plugin puts on it's containers could be considered private to the plugin.
It is possible for uses to write scripts etc to take advantage of the labels that older versions have used so I guess we should ensure that there are straightforward instructions about migration in (or linked to) the change log...
So, with that in mind, I would suggest that the issue or or we link to (when updating the change log file) should contain a before-and-after summary of label changes in its initial description so that users can easily adapt their scripts after upgrading the plugin.

As for loosing containers: a plugin upgrade requires a Jenkins restart; restarts will (normally) happen only once no build jobs are running; docker containers get terminated at the end of a build; so, under normal circumstances, we won't lose containers as there won't be any to lose.

i.e. I think we should be ok.

eaglerainbow · 2018-06-09T20:23:48Z

Ok, thanks for the feedback.
With https://github.com/eaglerainbow/docker-plugin/blob/f5083176c917ff73a70ec06500b3ab8224320db0/CHANGELOG.md I have adjusted the CHANGELOG.md in such a way that with the next (patch/minor) release a mapping from "old state" to "new state" is made available. Do you think that this is sufficient?

Besides that, any other comments, e.g. on coding level?
I checked FB, and it finds this PR acceptable...

pjdarton

Overall I agree with what's being done here, but we need to undo the code churn on the import statements and the changelog.

pjdarton · 2018-06-10T11:49:50Z

src/main/java/com/nirima/jenkins/plugins/docker/DockerContainerLabelKeys.java

+     * 
+     * Label keys defined in this interface already have this namespace prefixed.
+     */
+    static final String PLUGIN_LABEL_KEY_NAMESPACE = "com.nirima.jenkins.plugins.docker.";


I don't think this needs to be visible outside this class.

I would also question if it would, in fact, be better to use .class.getPackage().getCanonicalName() + "." instead of hard-coding the text.
(I can see arguments for & against; I'm not too fussed either way, just as long as both options have been considered)

I don't think this needs to be visible outside this class.

public interfaces in java do not permit to specify an attribute to be private. It'll mean that we would have to make it an abstract class, which also has a co-notation which I don't like (as we don't want to implement this artifact at all anyway).

On the other hand, we have the counter-example already in this plugin with com.nirima.jenkins.plugins.docker.utils.Consts. If we already have an approach for such cases, we should stick to one concept. I have adjusted it with 30c937b .

.class.getPackage().getCanonicalName() + "."

I did not think over that aspect too much yet. The only thing is that I always dislike, if constants change their value. That might happen, if someone is moving our own namespace around. However, on the other side, if somebody changes the namespace of this plugin, then it'll also be a good idea, if the constants will change (as most likely the "new version" of the plugin will be "something different").

I browsed a little through the coding. My impression is that it more tends to the concept of class-based reference than to absolute referencing. That is why I have adjusted the coding accordingly with 983fdc3.

pjdarton · 2018-06-10T11:58:13Z

CHANGELOG.md

@@ -1,5 +1,16 @@
 # Changelog

+## 1.1.x [draft only!]


I think this detailed explanation is best left to the PR description for this PR.
The changelog should just be one-line summaries with links to additional information as 99% of readers won't care about the details.

Also, I think it's best to leave updating the changelog itself to immediately after the official release because otherwise we might confuse people - the official wiki page links to this file in the master branch so the moment I merge changes it'll be visible to the masses, and folks will assume that if they have the latest release, they'll have all the changes listed.

That is wh I had marked it as "[draft only!]".
But I am ok with it that we remove the stuff in CHANGELOG.md and move it to a comment here.

I have done so with b058509

pjdarton · 2018-06-10T12:03:26Z

src/main/java/com/nirima/jenkins/plugins/docker/DockerTemplate.java

-import java.util.Collections;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Set;


I think you've allowed your IDE to re-arrange all the import statements here.
This makes it very difficult to see what changes have actually been made.

I'm not against changes that re-arrange the code for aesthetic reasons, but they should be kept separate from functional changes.

I will comply to your request, but still I think it's wrong:
You are right that I have set up my IDE to automatically adjust imports. Imports, IMO, are (usually) just syntactic sugar, which I, as developer, do not need to take care of (I have plenty other things to do). These kind of activities do not change anything on semantic level.
If, by chance of this PR, we also clean up the code and don't need to start the entire CI for such a small thing, then this is good (I'd understand your concerns, if I had changed something in the coding, e.g. changes on sequence of methods, or such-a-like).
Would you have approved a PR, with only these aesthetic changes, if I had sent it separately?

reverted with 4ae4722

The idea is to ensure that important changes are kept to a minimalist set of changed lines of code so that they're easy to merge and will only cause merge conflicts of there is an actual conflict of functionality.

Aesthetic changes are also necessary to keep the code from getting too ugly but they do tend to touch lots and lots of lines of code when they happen, so they're almost impossible to merge with anything, so it's important that nobody ever needs to, which means that these kinds of changes have to be kept apart from functionality changes.

i.e. if you find the code intolerably ugly when working, and can't bear the thought of leaving it like that, do a tidy-up PR first ... or afterwards ... but not at the same time ;-)

pjdarton · 2018-06-10T12:07:20Z

src/main/java/com/nirima/jenkins/plugins/docker/DockerTemplateBase.java

-import java.util.Map;
-
-import static org.apache.commons.lang.StringUtils.isEmpty;
-import static org.apache.commons.lang.StringUtils.trimToNull;


Import statement re-arrangement should not be part of this PR.
(Personally, I prefer to have static imports at the start too, but this is not what this PR is for)

Conflicts: CHANGELOG.md

eaglerainbow

all comments worked on. Back to you...

eaglerainbow · 2018-06-10T19:08:46Z

src/main/java/com/nirima/jenkins/plugins/docker/DockerTemplateBase.java

-import java.util.Map;
-
-import static org.apache.commons.lang.StringUtils.isEmpty;
-import static org.apache.commons.lang.StringUtils.trimToNull;


eaglerainbow · 2018-06-10T19:08:51Z

src/main/java/com/nirima/jenkins/plugins/docker/DockerTemplate.java

-import java.util.Collections;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Set;


I will comply to your request, but still I think it's wrong:
You are right that I have set up my IDE to automatically adjust imports. Imports, IMO, are (usually) just syntactic sugar, which I, as developer, do not need to take care of (I have plenty other things to do). These kind of activities do not change anything on semantic level.
If, by chance of this PR, we also clean up the code and don't need to start the entire CI for such a small thing, then this is good (I'd understand your concerns, if I had changed something in the coding, e.g. changes on sequence of methods, or such-a-like).
Would you have approved a PR, with only these aesthetic changes, if I had sent it separately?

reverted with 4ae4722

eaglerainbow · 2018-06-10T19:08:56Z

src/main/java/com/nirima/jenkins/plugins/docker/DockerContainerLabelKeys.java

+     * 
+     * Label keys defined in this interface already have this namespace prefixed.
+     */
+    static final String PLUGIN_LABEL_KEY_NAMESPACE = "com.nirima.jenkins.plugins.docker.";


I don't think this needs to be visible outside this class.

public interfaces in java do not permit to specify an attribute to be private. It'll mean that we would have to make it an abstract class, which also has a co-notation which I don't like (as we don't want to implement this artifact at all anyway).

On the other hand, we have the counter-example already in this plugin with com.nirima.jenkins.plugins.docker.utils.Consts. If we already have an approach for such cases, we should stick to one concept. I have adjusted it with 30c937b .

.class.getPackage().getCanonicalName() + "."

I did not think over that aspect too much yet. The only thing is that I always dislike, if constants change their value. That might happen, if someone is moving our own namespace around. However, on the other side, if somebody changes the namespace of this plugin, then it'll also be a good idea, if the constants will change (as most likely the "new version" of the plugin will be "something different").

I browsed a little through the coding. My impression is that it more tends to the concept of class-based reference than to absolute referencing. That is why I have adjusted the coding accordingly with 983fdc3.

eaglerainbow · 2018-06-10T19:08:58Z

CHANGELOG.md

@@ -1,5 +1,16 @@
 # Changelog

+## 1.1.x [draft only!]


That is wh I had marked it as "[draft only!]".
But I am ok with it that we remove the stuff in CHANGELOG.md and move it to a comment here.

I have done so with b058509

eaglerainbow added 5 commits June 8, 2018 23:33

Refactor: Extract Container label constants to own constant interface

b88bf32

Refactor: Remove redundancy from constant identifiers

a6b1f48

Marking as final

fe0e12b

Introducing namespace prefix for Docker label keys

26b1627

Improving links in javadoc

5134eb9

eaglerainbow added 2 commits June 9, 2018 22:18

Adding documentation old/new mapping in changelog

d640cde

Fix format issue in table

f508317

eaglerainbow mentioned this pull request Jun 9, 2018

[FEATURE] Docker Container&Slave Watchdog (new implementation) #658

Merged

pjdarton suggested changes Jun 10, 2018

View reviewed changes

eaglerainbow added 7 commits June 10, 2018 20:31

Remove early stage of comments on PR jenkinsci#660

b058509

Make constant PLUGIN_LABEL_KEY_NAMESPACE private

30c937b

Derive namespace from package name of DockerContainerLabelKeys class

983fdc3

Reverting aesthetic adjustment of import statements

4ae4722

Merge remote-tracking branch 'origin/labelNamespace' into labelNamespace

1d8110f

Conflicts: CHANGELOG.md

Fine tuning of reverting aesthetic import changes

44ac9ca

Re-inserting removed newline after imports

46c85d4

eaglerainbow commented Jun 10, 2018

View reviewed changes

pjdarton approved these changes Jun 10, 2018

View reviewed changes

pjdarton merged commit 907a314 into jenkinsci:master Jun 10, 2018

eaglerainbow deleted the labelNamespace branch June 10, 2018 19:59

eaglerainbow added a commit to eaglerainbow/docker-plugin that referenced this pull request Jun 10, 2018

Refactoring constants after PR jenkinsci#660 has been merged

fa99486

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Namespaces for Container Label Keys #660

Namespaces for Container Label Keys #660

eaglerainbow commented Jun 8, 2018 •

edited

Loading

eaglerainbow commented Jun 8, 2018

pjdarton commented Jun 9, 2018

eaglerainbow commented Jun 9, 2018

pjdarton left a comment

pjdarton Jun 10, 2018

eaglerainbow Jun 10, 2018

pjdarton Jun 10, 2018

eaglerainbow Jun 10, 2018

pjdarton Jun 10, 2018

eaglerainbow Jun 10, 2018 •

edited

Loading

pjdarton Jun 10, 2018

pjdarton Jun 10, 2018

eaglerainbow Jun 10, 2018

eaglerainbow left a comment

eaglerainbow Jun 10, 2018

eaglerainbow Jun 10, 2018 •

edited

Loading

eaglerainbow Jun 10, 2018

eaglerainbow Jun 10, 2018

Namespaces for Container Label Keys #660

Namespaces for Container Label Keys #660

Conversation

eaglerainbow commented Jun 8, 2018 • edited Loading

Mapping between old and new label keys

eaglerainbow commented Jun 8, 2018

pjdarton commented Jun 9, 2018

eaglerainbow commented Jun 9, 2018

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

eaglerainbow Jun 10, 2018 • edited Loading

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

eaglerainbow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eaglerainbow Jun 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eaglerainbow commented Jun 8, 2018 •

edited

Loading

eaglerainbow Jun 10, 2018 •

edited

Loading

eaglerainbow Jun 10, 2018 •

edited

Loading