Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kerogers-cloudbees
Copy link
Contributor

@kerogers-cloudbees kerogers-cloudbees commented Dec 10, 2020

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:

podTemplate(containers: [containerTemplate(args: 'cat', command: '/bin/sh -c', image: 'dockerImage', name: 'container', workingDir: '/home/jenkins/agent')]) {
    // some block
}

}
}

public FormValidation doCheckTimeoutSeconds(@QueryParameter int value) {
Copy link
Contributor Author

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.

Copy link
Member

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/>

Copy link
Member

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)

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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

Copy link
Member

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;
Copy link
Contributor Author

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.");
Copy link
Contributor Author

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.

@kerogers-cloudbees
Copy link
Contributor Author

I have it back to producing podTemplate(containers: [containerTemplate(args: 'cat', command: '/bin/sh -c', image: 'bbbbb', name: 'aaaaa', workingDir: '/home/jenkins/agent')]) { // some block } as the output again.

@Vlatombe Vlatombe changed the title [CPLT2-6334] Clean up the snippet generation for a container template. Clean up the snippet generation for a container template. Dec 23, 2020
@Vlatombe Vlatombe added the enhancement Improvements label Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements
Projects
None yet
2 participants