-
Notifications
You must be signed in to change notification settings - Fork 323
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.
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
Support setting additional labels on slave containers #747
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two main issues:
-
Naming
Choosing names for things is never as straightforward as it could be and, sadly, this is no exception 😬
Slave nodes (and templates that create them) already have a field for "labels", so having a field called "extraLabels" sounds like it's going to add to the labels field rather than the docker container labels.
I think we need a big rename here and call the new field "extraDockerLabels" rather than just "extraLabels" (plus extra text in the html help file).
Hopefully a big search & replace turningxtraLabels
toxtraDockerLabels
will do 95% of the work... -
Configuration round trip
The getter/setter pairing turns nulls to empties and hashCode/equals treats this as a change, resulting in a unit-test failure: https://ci.jenkins.io/job/Plugins/job/docker-plugin/view/change-requests/job/PR-747/1/testReport/
Either persist nulls as nulls and empties as empties ... or ensure that empties are always stored as nulls.
Personally, I quite like the idea of absent/unused configuration fields simply not appearing at all in the saved config (as it makes for smaller more efficient files), which means we need nulls when we're turned into XML.
src/main/java/com/nirima/jenkins/plugins/docker/DockerTemplateBase.java
Outdated
Show resolved
Hide resolved
src/main/java/com/nirima/jenkins/plugins/docker/DockerTemplateBase.java
Outdated
Show resolved
Hide resolved
src/main/java/com/nirima/jenkins/plugins/docker/DockerTemplateBase.java
Outdated
Show resolved
Hide resolved
src/main/java/com/nirima/jenkins/plugins/docker/DockerTemplateBase.java
Outdated
Show resolved
Hide resolved
...n/resources/com/nirima/jenkins/plugins/docker/DockerTemplateBase/help-extraLabelsString.html
Outdated
Show resolved
Hide resolved
src/main/resources/com/nirima/jenkins/plugins/docker/DockerTemplateBase/config.jelly
Outdated
Show resolved
Hide resolved
Thanks for the PR - enhancements such as this are always welcome 😀 I've done a review - please resolve the changes requested and ping me once you think it's there. Hint: If the automated build on |
8544d52
to
3e4fefe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the doCheckExtraDockerLabelsString method.
src/main/java/com/nirima/jenkins/plugins/docker/DockerTemplateBase.java
Outdated
Show resolved
Hide resolved
...urces/com/nirima/jenkins/plugins/docker/DockerTemplateBase/help-extraDockerLabelsString.html
Show resolved
Hide resolved
470ccee
to
019e659
Compare
019e659
to
e4fae0a
Compare
All changes done, can you please review? |
This patch adds an extraLabels fields to DockerTemplateBase, which is used to specify additional docker labels to set on slave containers.
This can be used for example to logically group containers, or to setup scheduling constraints on swarm.
See also: https://issues.jenkins-ci.org/browse/JENKINS-45227