Skip to content

Clean up the snippet generation for a container template. #917

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.

Sign up for GitHub

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

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

Clean up the snippet generation for a container template. #917

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0bda546
[CPLT2-6334] Clean up the snippet generation for a container template.
kerogers-cloudbees Dec 10, 2020
1a77924
[CPLT2-6334] Make liveness probe optional.
kerogers-cloudbees Dec 14, 2020
07cb7a8
[CPLT2-6334] Removed unused function. Removed unnecessary jelly block…
kerogers-cloudbees Jan 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 75 additions & 7 deletions src/main/java/org/csanchez/jenkins/plugins/kubernetes/ContainerLivenessProbe.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,27 @@
package org.csanchez.jenkins.plugins.kubernetes;

import hudson.Extension;
import hudson.Util;
import hudson.model.AbstractDescribableImpl;
import hudson.model.Descriptor;
import hudson.util.FormValidation;
import org.apache.commons.lang.StringUtils;
import org.jenkinsci.Symbol;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.QueryParameter;

import java.io.Serializable;

/**
* Created by fabricio.leotti on 26/04/17.
*/
public class ContainerLivenessProbe extends AbstractDescribableImpl<ContainerLivenessProbe> implements Serializable {
private String execArgs;
private int timeoutSeconds;
private int initialDelaySeconds;
private int failureThreshold;
private int periodSeconds;
private int successThreshold;
private String execArgs = DescriptorImpl.DEFAULT_EXEC_ARGS;
private int timeoutSeconds = DescriptorImpl.DEFAULT_TIMEOUT_SECONDS;
private int initialDelaySeconds = DescriptorImpl.DEFAULT_INITIAL_DELAY_SECONDS;
private int failureThreshold = DescriptorImpl.DEFAULT_FAILURE_THRESHOLD;
private int periodSeconds = DescriptorImpl.DEFAULT_PERIOD_SECONDS;
private int successThreshold = DescriptorImpl.DEFAULT_SUCCESS_THRESHOLD;

@DataBoundConstructor
public ContainerLivenessProbe(String execArgs, int timeoutSeconds, int initialDelaySeconds, int failureThreshold, int periodSeconds, int successThreshold) {
Expand All @@ -34,7 +38,7 @@ public String getExecArgs() {
}

public void setExecArgs(String execArgs) {
this.execArgs = execArgs;
this.execArgs = Util.fixEmpty(execArgs);
}

public int getTimeoutSeconds() {
Expand Down Expand Up @@ -96,5 +100,69 @@ public static class DescriptorImpl extends Descriptor<ContainerLivenessProbe> {
public String getDisplayName() {
return "Container Liveness Probe";
}

public static final String DEFAULT_EXEC_ARGS = "uptime";
public static final int DEFAULT_TIMEOUT_SECONDS = 1;
public static final int DEFAULT_INITIAL_DELAY_SECONDS = 0;
public static final int DEFAULT_PERIOD_SECONDS = 10;
public static final int DEFAULT_FAILURE_THRESHOLD = 3;
public static final int DEFAULT_SUCCESS_THRESHOLD = 1;

public FormValidation doCheckExecArgs(@QueryParameter String value) {
if (StringUtils.isEmpty(value)) {
return FormValidation.error("Liveness probe command is mandatory");
} else {
return FormValidation.ok();
}
}

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 ?

if (value < 1) {
return FormValidation.error("Minimum Timeout Seconds value is 1");
} else {
return FormValidation.ok();
}
}

public FormValidation doCheckInitialDelaySeconds(@QueryParameter int value) {
if (value < 0) {
return FormValidation.error("Minimum Initial Delay Seconds value is 0");
} else {
return FormValidation.ok();
}
}

public FormValidation doCheckPeriodSeconds(@QueryParameter int value) {
if (value < 1) {
return FormValidation.error("Minimum Period Seconds value is 1");
} else {
return FormValidation.ok();
}
}

public FormValidation doCheckFailureThreshold(@QueryParameter int value) {
if (value < 1) {
return FormValidation.error("Minimum Failure Threshold value is 1");
} else {
return FormValidation.ok();
}
}

public FormValidation doCheckSuccessThreshold(@QueryParameter int value) {
if (value < 1) {
return FormValidation.error("Minimum Success Threshold value is 1");
} else {
return FormValidation.ok();
}
}
}

public static ContainerLivenessProbe getDefault() {
return new ContainerLivenessProbe(DescriptorImpl.DEFAULT_EXEC_ARGS,
DescriptorImpl.DEFAULT_TIMEOUT_SECONDS,
DescriptorImpl.DEFAULT_INITIAL_DELAY_SECONDS,
DescriptorImpl.DEFAULT_FAILURE_THRESHOLD,
DescriptorImpl.DEFAULT_PERIOD_SECONDS,
DescriptorImpl.DEFAULT_SUCCESS_THRESHOLD);
}
}
32 changes: 20 additions & 12 deletions src/main/java/org/csanchez/jenkins/plugins/kubernetes/ContainerTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ public String getImage() {

@DataBoundSetter
public void setCommand(String command) {
this.command = command;
this.command = Util.fixEmpty(command);
}

public String getCommand() {
Expand All @@ -139,7 +139,7 @@ public String getCommand() {

@DataBoundSetter
public void setArgs(String args) {
this.args = args;
this.args = Util.fixEmpty(args);
}

public String getArgs() {
Expand Down Expand Up @@ -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.

}

@DataBoundSetter
public void setLivenessProbe(ContainerLivenessProbe livenessProbe) {
this.livenessProbe = livenessProbe;
this.livenessProbe = (livenessProbe == null || livenessProbe.equals(DescriptorImpl.defaultLivenessProbe()) ? null : livenessProbe);
}

public List<PortMapping> getPorts() {
Expand All @@ -244,7 +246,7 @@ public String getResourceRequestMemory() {

@DataBoundSetter
public void setResourceRequestMemory(String resourceRequestMemory) {
this.resourceRequestMemory = resourceRequestMemory;
this.resourceRequestMemory = Util.fixEmpty(resourceRequestMemory);
}

public String getResourceLimitMemory() {
Expand All @@ -253,16 +255,16 @@ public String getResourceLimitMemory() {

@DataBoundSetter
public void setResourceLimitMemory(String resourceLimitMemory) {
this.resourceLimitMemory = resourceLimitMemory;
this.resourceLimitMemory = Util.fixEmpty(resourceLimitMemory);
}

public String getResourceRequestCpu() {
return resourceRequestCpu;
}

@DataBoundSetter
public void setResourceRequestCpu(String resourceRequestCpu) {
this.resourceRequestCpu = resourceRequestCpu;
public void setResourceRequestCpu(String resourceRequestCpu){
this.resourceRequestCpu = Util.fixEmpty(resourceRequestCpu);
}

public String getResourceLimitCpu() {
Expand All @@ -271,7 +273,7 @@ public String getResourceLimitCpu() {

@DataBoundSetter
public void setResourceLimitCpu(String resourceLimitCpu) {
this.resourceLimitCpu = resourceLimitCpu;
this.resourceLimitCpu = Util.fixEmpty(resourceLimitCpu);
}

public String getResourceRequestEphemeralStorage() {
Expand All @@ -280,7 +282,7 @@ public String getResourceRequestEphemeralStorage() {

@DataBoundSetter
public void setResourceRequestEphemeralStorage(String resourceRequestEphemeralStorage) {
this.resourceRequestEphemeralStorage = resourceRequestEphemeralStorage;
this.resourceRequestEphemeralStorage = Util.fixEmpty(resourceRequestEphemeralStorage);
}

public String getResourceLimitEphemeralStorage() {
Expand All @@ -289,7 +291,7 @@ public String getResourceLimitEphemeralStorage() {

@DataBoundSetter
public void setResourceLimitEphemeralStorage(String resourceLimitEphemeralStorage) {
this.resourceLimitEphemeralStorage = resourceLimitEphemeralStorage;
this.resourceLimitEphemeralStorage = Util.fixEmpty(resourceLimitEphemeralStorage);
}


Expand All @@ -299,7 +301,7 @@ public String getShell() {

@DataBoundSetter
public void setShell(String shell) {
this.shell = shell;
this.shell = Util.fixEmpty(shell);
}

public Map<String,Object> getAsArgs() {
Expand Down Expand Up @@ -330,6 +332,9 @@ public List<? extends Descriptor> getEnvVarsDescriptors() {
}

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.

}
if(!PodTemplateUtils.validateContainerName(value)) {
return FormValidation.error(Messages.RFC1123_error(value));
}
Expand All @@ -350,6 +355,9 @@ public FormValidation doCheckImage(@QueryParameter String value) {
public String getWorkingDir() {
return DEFAULT_WORKING_DIR;
}
public static final ContainerLivenessProbe defaultLivenessProbe() {
return ContainerLivenessProbe.getDefault();
}
}

@Override
Expand Down
14 changes: 7 additions & 7 deletions ...ain/resources/org/csanchez/jenkins/plugins/kubernetes/ContainerLivenessProbe/config.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,26 @@
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout"
xmlns:t="/lib/hudson" xmlns:f="/lib/form">
<f:entry field="execArgs" title="${%Exec action}">
<f:textbox/>
<f:textbox default="${descriptor.DEFAULT_EXEC_ARGS}"/>
</f:entry>

<f:entry field="initialDelaySeconds" title="${%Initial Delay Seconds}">
<f:textbox/>
<f:textbox default="${descriptor.DEFAULT_INITIAL_DELAY_SECONDS}"/>
</f:entry>

<f:entry field="timeoutSeconds" title="${%Timeout Seconds}">
<f:textbox/>
<f:textbox default="${descriptor.DEFAULT_TIMEOUT_SECONDS}"/>
</f:entry>

<f:entry field="failureThreshold" title="${%Failure Threshold}">
<f:textbox/>
<f:textbox default="${descriptor.DEFAULT_FAILURE_THRESHOLD}"/>
</f:entry>

<f:entry field="periodSeconds" title="${%Period Seconds}">
<f:textbox/>
<f:textbox default="${descriptor.DEFAULT_PERIOD_SECONDS}"/>
</f:entry>

<f:entry field="successThreshold" title="${%Success Threshold}">
<f:textbox/>
<f:textbox default="${descriptor.DEFAULT_SUCCESS_THRESHOLD}"/>
</f:entry>
</j:jelly>
</j:jelly>