-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[JENKINS-58670] Pod Labels UI #551
Conversation
For clusters that run multiple jenkins instance in the same namespace and also want to limit the number of slave pods by instance, being able to manage the pod labels is important. Because there was no UI and thus no DataBoundSetter any labels set via an init.groovy.d script or script console would get wiped out on any config change. This changeset introduces a `labelsStr` property which is used to bind the data from the UI form. The string format is a simple key=value pair separated by whitespace. Do to the restricted syntax and character set defined by kubernetes parsing of the labels is pretty straightforward.
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.
It's a great idea, though it would be preferrable in my opinion to use the same pattern as what is used for pod annotations rather than introduce a different way to provide the input.
<p>Examples: | ||
<ul> | ||
<li><code>jenkins=slave</code></li> | ||
<li><code>app.kubernetes.io/managed-by=jenkins app.kubernetes.io/instance=dev</code></li> |
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.
According to https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/, app.kubernetes.io/instance
is a unique name. app.kubernetes.io/part-of
seems more appropriate.
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.
right on, I'll refactor to follow that pattern.
Add Describable PodLabel for configuring a list of labels to add to all slave Pods created by the plugin.
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.
Looks good overall, just missing the backward compatibility.
@@ -116,7 +118,7 @@ | |||
private int retentionTimeout = DEFAULT_RETENTION_TIMEOUT_MINUTES; | |||
private int connectTimeout; | |||
private int readTimeout; | |||
private Map<String, String> labels; |
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.
To achieve backward compatibility, you need to keep the old field as @deprecated / transient
, then migrate the data in readResolve()
method.
Added back the the original set/get labels of a map and marked them as deprecated. The new bean property is named "podLabels". Included a migration from the deprecated labels map to the pod labels list in the readResolve method.
* set old labels field as transient * Fix copy constructor and a test * Simplify getPodLabels * Update KubernetesCloud#toString() for debug * Implement PodLabel#toString() for debug * Update readResolve to initialize the new field if null
@@ -118,8 +118,8 @@ | |||
private int readTimeout; | |||
/** @deprecated Stored as a list of PodLabels */ | |||
@Deprecated | |||
private Map<String, String> labels; | |||
private List<PodLabel> podLabels = new ArrayList<PodLabel>(); | |||
private transient Map<String, String> labels; |
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.
@Vlatombe Thanks for catching that, I had it in there and removed it when I was testing.
JENKINS-58670
For clusters that run multiple jenkins instance in the same namespace
and also want to limit the number of slave pods by instance, being
able to manage the pod labels is important.
Because there was no UI and thus no DataBoundSetter any labels
set via an init.groovy.d script or script console would get wiped out
on any config change.
This changeset introduces a
labelsStr
property which is used tobind the data from the UI form. The string format is a simple key=value
pair separated by whitespace. Do to the restricted syntax and character
set defined by kubernetes parsing of the labels is pretty straightforward.