-
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
Clean up the snippet generation for a container template. #917
base: master
Are you sure you want to change the base?
Clean up the snippet generation for a container template. #917
Conversation
} | ||
} | ||
|
||
public FormValidation doCheckTimeoutSeconds(@QueryParameter int value) { |
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.
interestingly, if you put a string that isn't a number into the field, the form validation doesn't happen.
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.
You can use <f:number/>
instead of <f:textbox/>
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.
And use FormValidation.validatePositiveInteger(value)
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.
interesting, FormValidation.validatePositiveInteger()
takes a string as the argument. I'd really rather not rework this class to maintain strings and integers for these values.
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.
Ah yes because it works from a textbox, so it expects a string as input.
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.
The validation method can use string but the rest can use int just fine
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.
@kerogers-cloudbees Could you rework the validation methods with @QueryParameter int value
?
@@ -222,11 +222,13 @@ public void setEnvVars(List<TemplateEnvVar> envVars) { | |||
} | |||
|
|||
|
|||
public ContainerLivenessProbe getLivenessProbe() { return livenessProbe; } | |||
public ContainerLivenessProbe getLivenessProbe() { | |||
return livenessProbe == null ? DescriptorImpl.defaultLivenessProbe() : livenessProbe; |
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.
I realize this means all containers will have a liveness probe.
@@ -330,6 +332,9 @@ public String getDisplayName() { | |||
} | |||
|
|||
public FormValidation doCheckName(@QueryParameter String value) { | |||
if (StringUtils.isEmpty(value)) { | |||
return FormValidation.warning("Container name is mandatory."); |
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.
The image check uses FormValidation.ok()
for this, but it really doesn't stand out on the page. Maybe someone from UX can take a quick look and make a judgement on what we should use.
Also this warning appears as soon as you add a container template in the form, it doesn't wait for the text box to gain and lose focus.
src/main/java/org/csanchez/jenkins/plugins/kubernetes/ContainerLivenessProbe.java
Outdated
Show resolved
Hide resolved
I have it back to producing |
src/main/resources/org/csanchez/jenkins/plugins/kubernetes/ContainerTemplate/config.jelly
Outdated
Show resolved
Hide resolved
src/main/java/org/csanchez/jenkins/plugins/kubernetes/ContainerTemplate.java
Outdated
Show resolved
Hide resolved
… following optionalProperty.
Cleaned up the defaults for Container Template and Container Liveness Probe to minimize the output of a generated snippet for pod templates.
The output with the mandatory fields and the ones we couldn't risk changing: