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.

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

Jump to bottom

[JENKINS-44285] Tool Location overwrites are not preserved #318

Merged
merged 15 commits into from
Jul 16, 2018
Merged

[JENKINS-44285] Tool Location overwrites are not preserved #318

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
d0f75c1
JENKINS-44285
aytuncbeken Apr 25, 2018
6d27033
JENKINS-44285
aytuncbeken Apr 26, 2018
ddec66f
Merge branch 'master' into JENKINS-44285
carlossg May 22, 2018
960c0f4
[JENKINS-44285] Add test for nodeProperties save
carlossg May 22, 2018
8d95d98
[JENKINS-44285] Tool Location overwrites are not preserved - Reviewed
aytuncbeken Jun 23, 2018
7c25271
Merge branch 'JENKINS-44285' of https://github.com/aytuncbeken/kubern…
aytuncbeken Jun 23, 2018
9acb62c
[JENKINS-44285] Tool Location overwrites are not preserved
aytuncbeken Jun 23, 2018
c6f1df5
[JENKINS-44285] Tool Location overwrites are not preserved
aytuncbeken Jun 23, 2018
aa0a515
Merge branch 'master' into JENKINS-44285
aytuncbeken Jun 23, 2018
cb87010
[JENKINS-44285] Tool Location overwrites are not preserved
aytuncbeken Jun 23, 2018
0d18be8
[JENKINS-44285] Tool Location overwrites are not preserved
aytuncbeken Jun 26, 2018
a89692d
[JENKINS-44285] Tool Location overwrites are not preserved
aytuncbeken Jul 14, 2018
e9fbcb9
Merge branch 'master' of https://github.com/jenkinsci/kubernetes-plug…
aytuncbeken Jul 14, 2018
6b09e00
NodeProperties implemented As DescribableList as in Jenkins Slave
aytuncbeken Jul 15, 2018
0c0e489
Reformat
carlossg Jul 16, 2018
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
15 changes: 13 additions & 2 deletions src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

import java.util.Map;
import java.util.Optional;
import java.util.Set;
Expand All @@ -12,6 +13,7 @@

import javax.annotation.Nonnull;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import org.apache.commons.lang.StringUtils;
import org.csanchez.jenkins.plugins.kubernetes.model.TemplateEnvVar;
import org.csanchez.jenkins.plugins.kubernetes.volumes.PodVolume;
Expand Down Expand Up @@ -110,7 +112,8 @@ public class PodTemplate extends AbstractDescribableImpl<PodTemplate> implements

private List<PodImagePullSecret> imagePullSecrets = new ArrayList<PodImagePullSecret>();

private transient List<ToolLocationNodeProperty> nodeProperties;
@SuppressFBWarnings(value = "SE_BAD_FIELD")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this actually needed ? doesn't fail without it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I indeed get a FindBugs. However ignoring it feels like hack.

In Jenkins core, this is handled using a DescribableList (see here). I would recommend sticking to the same pattern.

private List<ToolLocationNodeProperty> nodeProperties = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on upgrade this will be null because the xml does not have the option nodeProperties entry, it should be checked in getNodeProperties


private String yaml;

Expand All @@ -135,6 +138,7 @@ public PodTemplate(PodTemplate from) {
this.setVolumes(from.getVolumes());
this.setWorkspaceVolume(from.getWorkspaceVolume());
this.setYaml(from.getYaml());
this.setNodeProperties(from.getNodeProperties());
}

@Deprecated
Expand Down Expand Up @@ -476,7 +480,14 @@ public void setImagePullSecrets(List<PodImagePullSecret> imagePullSecrets) {

@DataBoundSetter
public void setNodeProperties(List<ToolLocationNodeProperty> nodeProperties){
this.nodeProperties = nodeProperties;
if( nodeProperties != null){
this.nodeProperties.clear();
this.addNodeProperties(nodeProperties);
}
}

public void addNodeProperties(List<ToolLocationNodeProperty> nodeProperties){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this method?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be inlined.

this.nodeProperties.addAll(nodeProperties);
}

@Nonnull
Expand Down
6 changes: 5 additions & 1 deletion src/main/resources/org/csanchez/jenkins/plugins/kubernetes/PodTemplate/config.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,11 @@
<f:textbox/>
</f:entry>

<f:descriptorList title="${%Node Properties}" descriptors="${h.getNodePropertyDescriptors(descriptor.clazz)}" field="nodeProperties" />
<!-- descriptorList is not working somehow, repeatableProperty has same functionality, just ui changes a little -->
<f:entry title="${%Tool Locations}">
<f:repeatableProperty field="nodeProperties" noAddButton="false" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this gives a weird UI with 2 clicks to get to the data

screenshot 2018-05-22 15 14 02

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Vlatombe any suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd better stick to the existing one.

</f:entry>

<f:block>
<table>
<f:optionalBlock title="${%Use custom workspace volume}" field="customWorkspaceVolumeEnabled" inline="true">
Expand Down
22 changes: 21 additions & 1 deletion src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@
import com.cloudbees.plugins.credentials.SystemCredentialsProvider;
import com.cloudbees.plugins.credentials.impl.UsernamePasswordCredentialsImpl;

import hudson.model.Node;
import hudson.plugins.git.GitTool;
import hudson.slaves.NodeProperty;
import hudson.tools.ToolLocationNodeProperty;
import hudson.tools.ToolLocationNodeProperty.ToolLocation;
import hudson.util.Secret;

/**
Expand All @@ -62,8 +67,8 @@ public class KubernetesTest {

@Before
public void before() throws Exception {
cloud = r.jenkins.clouds.get(KubernetesCloud.class);
r.configRoundtrip();
cloud = r.jenkins.clouds.get(KubernetesCloud.class);
}

@Test
Expand All @@ -88,6 +93,21 @@ public void upgradeFrom_0_12() throws Exception {
new KeyValueEnvVar("pod_b_key", "pod_b_value")), templates.get(0).getEnvVars());
}

@Test
@LocalData()
public void upgradeFrom_0_10() throws Exception {
List<PodTemplate> templates = cloud.getTemplates();
PodTemplate template = templates.get(0);
List<ToolLocationNodeProperty> nodeProperties = template.getNodeProperties();
assertEquals(1, nodeProperties.size());
ToolLocationNodeProperty property = (ToolLocationNodeProperty) nodeProperties.get(0);
assertEquals(1, property.getLocations().size());
ToolLocation location = property.getLocations().get(0);
assertEquals("Default", location.getName());
assertEquals("/custom/path", location.getHome());
assertEquals(GitTool.class, location.getType().clazz);
}

@Test
@LocalData()
public void upgradeFrom_0_8() throws Exception {
Expand Down
20 changes: 20 additions & 0 deletions src/test/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,13 @@
import static org.hamcrest.Matchers.*;
import static org.junit.Assert.*;

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import hudson.tools.ToolLocationNodeProperty;
import org.csanchez.jenkins.plugins.kubernetes.model.KeyValueEnvVar;
import org.csanchez.jenkins.plugins.kubernetes.model.SecretEnvVar;
import org.csanchez.jenkins.plugins.kubernetes.volumes.HostPathVolume;
Expand Down Expand Up @@ -473,4 +475,22 @@ public void shouldSubstituteMultipleEnvVarsAndNotUseDefaultsForMissing() {
properties.put("key2", "value2");
assertEquals("value1 or value2 or ${key3}", substitute("${key1} or ${key2} or ${key3}", properties, "defaultValue"));
}

@Test
public void shouldCompineAllToolLocations()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

{
PodTemplate podTemplate1 = new PodTemplate();
List<ToolLocationNodeProperty> nodeProperties1 = new ArrayList<>();
nodeProperties1.add(new ToolLocationNodeProperty(new ToolLocationNodeProperty.ToolLocation("toolKey1@Test","toolHome1")));
podTemplate1.setNodeProperties(nodeProperties1);

PodTemplate podTemplate2 = new PodTemplate();
List<ToolLocationNodeProperty> nodeProperties2 = new ArrayList<>();
nodeProperties2.add(new ToolLocationNodeProperty(new ToolLocationNodeProperty.ToolLocation("toolKey2@Test","toolHome2")));
podTemplate2.setNodeProperties(nodeProperties2);

PodTemplate result = combine(podTemplate1,podTemplate2);

assertThat(result.getNodeProperties(), hasItems(nodeProperties1.get(0),nodeProperties2.get(0)));
}
}
91 changes: 91 additions & 0 deletions ...ources/org/csanchez/jenkins/plugins/kubernetes/KubernetesTest/upgradeFrom_0_10/config.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
<?xml version='1.0' encoding='UTF-8'?>
<hudson>
<disabledAdministrativeMonitors/>
<version>2.7.4</version>
<numExecutors>2</numExecutors>
<mode>NORMAL</mode>
<useSecurity>true</useSecurity>
<authorizationStrategy class="hudson.security.AuthorizationStrategy$Unsecured"/>
<securityRealm class="hudson.security.SecurityRealm$None"/>
<disableRememberMe>false</disableRememberMe>
<projectNamingStrategy class="jenkins.model.ProjectNamingStrategy$DefaultProjectNamingStrategy"/>
<workspaceDir>${JENKINS_HOME}/workspace/${ITEM_FULLNAME}</workspaceDir>
<buildsDir>${ITEM_ROOTDIR}/builds</buildsDir>
<jdks/>
<viewsTabBar class="hudson.views.DefaultViewsTabBar"/>
<myViewsTabBar class="hudson.views.DefaultMyViewsTabBar"/>
<clouds>
<org.csanchez.jenkins.plugins.kubernetes.KubernetesCloud plugin="kubernetes@0.10">
<name>kubernetes</name>
<templates>
<org.csanchez.jenkins.plugins.kubernetes.PodTemplate>
<inheritFrom></inheritFrom>
<name>maven</name>
<instanceCap>2147483647</instanceCap>
<idleMinutes>0</idleMinutes>
<label>maven</label>
<nodeSelector></nodeSelector>
<volumes/>
<containers>
<org.csanchez.jenkins.plugins.kubernetes.ContainerTemplate>
<name>maven</name>
<image>maven:alpine</image>
<privileged>false</privileged>
<alwaysPullImage>false</alwaysPullImage>
<workingDir>/home/jenkins</workingDir>
<command>/bin/sh -c</command>
<args>cat</args>
<ttyEnabled>true</ttyEnabled>
<resourceRequestCpu></resourceRequestCpu>
<resourceRequestMemory></resourceRequestMemory>
<resourceLimitCpu></resourceLimitCpu>
<resourceLimitMemory></resourceLimitMemory>
<envVars/>
</org.csanchez.jenkins.plugins.kubernetes.ContainerTemplate>
</containers>
<envVars/>
<annotations/>
<imagePullSecrets/>
<nodeProperties>
<hudson.tools.ToolLocationNodeProperty>
<locations>
<hudson.tools.ToolLocationNodeProperty_-ToolLocation>
<type>hudson.plugins.git.GitTool$DescriptorImpl</type>
<name>Default</name>
<home>/custom/path</home>
</hudson.tools.ToolLocationNodeProperty_-ToolLocation>
</locations>
</hudson.tools.ToolLocationNodeProperty>
</nodeProperties>
</org.csanchez.jenkins.plugins.kubernetes.PodTemplate>
</templates>
<serverUrl>http://example.com</serverUrl>
<skipTlsVerify>false</skipTlsVerify>
<namespace>default</namespace>
<jenkinsUrl></jenkinsUrl>
<containerCap>10</containerCap>
<retentionTimeout>5</retentionTimeout>
<connectTimeout>0</connectTimeout>
<readTimeout>0</readTimeout>
</org.csanchez.jenkins.plugins.kubernetes.KubernetesCloud>
</clouds>
<quietPeriod>5</quietPeriod>
<scmCheckoutRetryCount>0</scmCheckoutRetryCount>
<views>
<hudson.model.AllView>
<owner class="hudson" reference="../../.."/>
<name>All</name>
<filterExecutors>false</filterExecutors>
<filterQueue>false</filterQueue>
<properties class="hudson.model.View$PropertyList"/>
</hudson.model.AllView>
</views>
<primaryView>All</primaryView>
<slaveAgentPort>-1</slaveAgentPort>
<label></label>
<crumbIssuer class="hudson.security.csrf.DefaultCrumbIssuer">
<excludeClientIPFromCrumb>false</excludeClientIPFromCrumb>
</crumbIssuer>
<nodeProperties/>
<globalNodeProperties/>
</hudson>