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

aytuncbeken · 2018-04-26T17:20:13Z

SuppressFBWarning for SE_BAD_FIELD Tool location was not preserved when configuration is saved. Configuration ui is changed in jelly. NodeProperties is initialized in definition. Add NodeProperties method added.

Tool location was not preserved when configuration is saved. Configuration ui is changed in jelly. NodeProperties is initialized in definition. Add NodeProperties method added.

SuppressFBWarning for SE_BAD_FIELD Tool location was not preserved when configuration is saved. Configuration ui is changed in jelly. NodeProperties is initialized in definition. Add NodeProperties method added.

aytuncbeken · 2018-05-02T06:03:32Z

Hi Carlos, Could you please check the pull request. Thank you.

carlossg · 2018-05-22T13:15:25Z

src/main/resources/org/csanchez/jenkins/plugins/kubernetes/PodTemplate/config.jelly

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


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

@Vlatombe any suggestions?

I'd better stick to the existing one.

carlossg · 2018-05-22T13:23:15Z

src/test/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtilsTest.java

@@ -448,4 +450,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()


carlossg · 2018-05-22T13:26:58Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplate.java

@@ -110,7 +112,8 @@

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

-    private transient List<ToolLocationNodeProperty> nodeProperties;
+    @SuppressFBWarnings(value = "SE_BAD_FIELD")
+    private List<ToolLocationNodeProperty> nodeProperties = new ArrayList<>();


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

carlossg · 2018-05-22T13:28:04Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplate.java

+        }
+    }
+
+    public void addNodeProperties(List<ToolLocationNodeProperty> nodeProperties){


do we need this method?

Should be inlined.

carlossg · 2018-05-22T13:54:57Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplate.java

@@ -110,7 +112,8 @@

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

-    private transient List<ToolLocationNodeProperty> nodeProperties;
+    @SuppressFBWarnings(value = "SE_BAD_FIELD")


is this actually needed ? doesn't fail without it

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.

aytuncbeken · 2018-05-28T05:56:43Z

Hi,

Thank you for the review, i will look asap.

…etes-plugin into JENKINS-44285

NodeProperties implemented As DescribableList as in Jenkins Slave UI preserved as in Node Configuration SuppressFBWarning for SE_BAD_FIELD setNodeProperties throws IOException, throws added to methods which related.

NodeProperties implemented As DescribableList as in Jenkins Slave UI preserved as in Node Configuration SuppressFBWarning for SE_BAD_FIELD setNodeProperties throws IOException, throws added to methods which related. Comments added.

# Conflicts: # src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtils.java # src/test/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtilsTest.java

NodeProperties implemented As DescribableList as in Jenkins Slave UI preserved as in Node Configuration SuppressFBWarning for SE_BAD_FIELD setNodeProperties throws IOException, throws added to methods which related. Comments added.

NodeProperties implemented As DescribableList as in Jenkins Slave UI preserved as in Node Configuration DescribableList implemented as PodTemplateToolLocation as Serializable Comments added.

aytuncbeken · 2018-07-05T06:42:43Z

Hi All,
Had chance to check this pull request ? Do you have any reviews ?

carlossg · 2018-07-05T08:20:23Z

The new throws IOException should be removed from most if not all methods.
Public methods changed should be deprecated instead of changed.
I will take another look once that is fixed and master is merged

NodeProperties implemented As DescribableList as in Jenkins Slave UI preserved as in Node Configuration DescribableList implemented as PodTemplateToolLocation as Serializable Comments added. Unnecessary throws Removed

… into JENKINS-44285 # Conflicts: # src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtils.java

aytuncbeken · 2018-07-14T15:07:06Z

Hi Carlos,
could you please check , thanks.

carlossg · 2018-07-15T11:25:18Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java

@@ -512,7 +512,7 @@ public PodTemplate getTemplate(@CheckForNull Label label) {
     * @param podTemplate the pod template to unwrap.
     * @return the unwrapped pod template
     */
-    public PodTemplate getUnwrappedTemplate(PodTemplate podTemplate) {
+    public PodTemplate getUnwrappedTemplate(PodTemplate podTemplate) throws IOException {


should not throw IOException

carlossg · 2018-07-15T11:25:34Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplate.java


    private String yaml;

    @DataBoundConstructor
    public PodTemplate() {
    }

-    public PodTemplate(PodTemplate from) {
+    public PodTemplate(PodTemplate from) throws IOException {


should not throw IOException

carlossg · 2018-07-15T11:29:08Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateToolLocation.java

@@ -0,0 +1,36 @@
+package org.csanchez.jenkins.plugins.kubernetes;


can you add the MIT license header please

carlossg · 2018-07-15T11:30:29Z

src/test/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtilsTest.java

@@ -215,7 +219,7 @@ public void shouldUnwrapParent() {
    }

    @Test
-    public void shouldDropNoDataWhenIdentical() {
+    public void shouldDropNoDataWhenIdentical()  {


extra whitespaces

UI preserved as in Node Configuration DescribableList implemented as PodTemplateToolLocation as Serializable Comments added. Unnecessary throws Removed White Spaces removed Mit License Added

aytuncbeken · 2018-07-15T19:26:15Z

Made the changes.

aytuncbeken added 2 commits April 25, 2018 12:31

JENKINS-44285

d0f75c1

Tool location was not preserved when configuration is saved. Configuration ui is changed in jelly. NodeProperties is initialized in definition. Add NodeProperties method added.

JENKINS-44285

6d27033

SuppressFBWarning for SE_BAD_FIELD Tool location was not preserved when configuration is saved. Configuration ui is changed in jelly. NodeProperties is initialized in definition. Add NodeProperties method added.

carlossg changed the title ~~Jenkins 44285~~ [JENKINS-44285] Tool Location overwrites are not preserved May 21, 2018

carlossg added 2 commits May 22, 2018 12:37

Merge branch 'master' into JENKINS-44285

ddec66f

[JENKINS-44285] Add test for nodeProperties save

960c0f4

carlossg requested changes May 22, 2018

View reviewed changes

aytuncbeken added 2 commits June 23, 2018 16:10

[JENKINS-44285] Tool Location overwrites are not preserved - Reviewed

8d95d98

Merge branch 'JENKINS-44285' of https://github.com/aytuncbeken/kubern…

7c25271

…etes-plugin into JENKINS-44285

aytuncbeken closed this Jun 23, 2018

aytuncbeken added 4 commits June 23, 2018 20:19

[JENKINS-44285] Tool Location overwrites are not preserved

9acb62c

NodeProperties implemented As DescribableList as in Jenkins Slave UI preserved as in Node Configuration SuppressFBWarning for SE_BAD_FIELD setNodeProperties throws IOException, throws added to methods which related.

Merge branch 'master' into JENKINS-44285

aa0a515

# Conflicts: # src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtils.java # src/test/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtilsTest.java

carlossg mentioned this pull request Jun 25, 2018

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

Closed

aytuncbeken reopened this Jun 26, 2018

[JENKINS-44285] Tool Location overwrites are not preserved

0d18be8

NodeProperties implemented As DescribableList as in Jenkins Slave UI preserved as in Node Configuration DescribableList implemented as PodTemplateToolLocation as Serializable Comments added.

aytuncbeken added 2 commits July 14, 2018 14:13

[JENKINS-44285] Tool Location overwrites are not preserved

a89692d

NodeProperties implemented As DescribableList as in Jenkins Slave UI preserved as in Node Configuration DescribableList implemented as PodTemplateToolLocation as Serializable Comments added. Unnecessary throws Removed

Merge branch 'master' of https://github.com/jenkinsci/kubernetes-plugin…

e9fbcb9

… into JENKINS-44285 # Conflicts: # src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtils.java

carlossg requested changes Jul 15, 2018

View reviewed changes

NodeProperties implemented As DescribableList as in Jenkins Slave

6b09e00

UI preserved as in Node Configuration DescribableList implemented as PodTemplateToolLocation as Serializable Comments added. Unnecessary throws Removed White Spaces removed Mit License Added

Reformat

0c0e489

carlossg approved these changes Jul 16, 2018

View reviewed changes

carlossg merged commit 3599d4d into jenkinsci:master Jul 16, 2018

jglick mentioned this pull request Oct 2, 2020

PodTemplateUtils#combine shouldn't have a side-effect on the parent pod template #853

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

aytuncbeken commented Apr 26, 2018

aytuncbeken commented May 2, 2018

carlossg May 22, 2018

carlossg May 22, 2018

Vlatombe May 22, 2018

carlossg May 22, 2018

carlossg May 22, 2018

carlossg May 22, 2018

Vlatombe May 22, 2018

carlossg May 22, 2018

Vlatombe May 22, 2018

aytuncbeken commented May 28, 2018

aytuncbeken commented Jul 5, 2018

carlossg commented Jul 5, 2018

aytuncbeken commented Jul 14, 2018

carlossg Jul 15, 2018

carlossg Jul 15, 2018

carlossg Jul 15, 2018

carlossg Jul 15, 2018

aytuncbeken commented Jul 15, 2018

		@@ -0,0 +1,36 @@
		package org.csanchez.jenkins.plugins.kubernetes;

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

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

Conversation

aytuncbeken commented Apr 26, 2018

aytuncbeken commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aytuncbeken commented May 28, 2018

aytuncbeken commented Jul 5, 2018

carlossg commented Jul 5, 2018

aytuncbeken commented Jul 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aytuncbeken commented Jul 15, 2018