Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,19 @@
import static org.hamcrest.core.Is.is;
import static org.junit.jupiter.api.Assertions.assertEquals;

import hudson.node_monitors.DiskSpaceMonitorNodeProperty;
import hudson.slaves.EnvironmentVariablesNodeProperty;
import hudson.slaves.NodeProperty;
import hudson.slaves.NodePropertyDescriptor;
import hudson.tools.ToolLocationNodeProperty;
import hudson.util.DescribableList;
import io.jenkins.plugins.casc.misc.ConfiguredWithCode;
import io.jenkins.plugins.casc.misc.JenkinsConfiguredWithCodeRule;
import io.jenkins.plugins.casc.misc.junit.jupiter.WithJenkinsConfiguredWithCode;
import io.jenkins.plugins.casc.model.CNode;
import java.util.Iterator;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
import jenkins.model.Jenkins;
import org.junit.jupiter.api.Test;
Expand All @@ -30,14 +34,37 @@ void configure(JenkinsConfiguredWithCodeRule j) {

DescribableList<NodeProperty<?>, NodePropertyDescriptor> nodeProperties = jenkins.getGlobalNodeProperties();

Set<Map.Entry<String, String>> entries = ((EnvironmentVariablesNodeProperty) nodeProperties.get(0))
assertEquals(3, nodeProperties.size());

Set<Map.Entry<String, String>> envVars = ((EnvironmentVariablesNodeProperty)
nodeProperties.get(EnvironmentVariablesNodeProperty.class))
.getEnvVars()
.entrySet();
assertEquals(1, entries.size());
assertEquals(3, envVars.size());

Map.Entry<String, String> envVar = entries.iterator().next();
Iterator<Entry<String, String>> iterator = envVars.iterator();
Map.Entry<String, String> envVar = iterator.next();
assertEquals("FOO", envVar.getKey());
assertEquals("BAR", envVar.getValue());

envVar = iterator.next();
assertEquals("FOO2", envVar.getKey());
assertEquals("", envVar.getValue());

envVar = iterator.next();
assertEquals("FOO3", envVar.getKey());
assertEquals("", envVar.getValue());

DiskSpaceMonitorNodeProperty diskSpace = nodeProperties.get(DiskSpaceMonitorNodeProperty.class);
assertEquals("1GiB", diskSpace.getFreeDiskSpaceThreshold());
assertEquals("2GiB", diskSpace.getFreeDiskSpaceWarningThreshold());
assertEquals("1GiB", diskSpace.getFreeTempSpaceThreshold());
assertEquals("2GiB", diskSpace.getFreeTempSpaceWarningThreshold());

ToolLocationNodeProperty toolLocations = nodeProperties.get(ToolLocationNodeProperty.class);
assertEquals(1, toolLocations.getLocations().size());
assertEquals("Default", toolLocations.getLocations().get(0).getName());
assertEquals("/home/user/bin/git", toolLocations.getLocations().get(0).getHome());
}

@Test
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,18 @@
jenkins:
globalNodeProperties:
- diskSpaceMonitor:
freeDiskSpaceThreshold: "1GiB"
freeDiskSpaceWarningThreshold: "2GiB"
freeTempSpaceThreshold: "1GiB"
freeTempSpaceWarningThreshold: "2GiB"
- envVars:
env:
- key: FOO
value: BAR
- key: FOO2
value: ""
- key: FOO3
- toolLocation:
locations:
- home: "/home/user/bin/git"
key: "hudson.plugins.git.GitTool$DescriptorImpl@Default"
Original file line number Diff line number Diff line change
@@ -1,4 +1,15 @@
- diskSpaceMonitor:
freeDiskSpaceThreshold: "1GiB"
freeDiskSpaceWarningThreshold: "2GiB"
freeTempSpaceThreshold: "1GiB"
freeTempSpaceWarningThreshold: "2GiB"
- envVars:
env:
- key: "FOO"
value: "BAR"
- key: "FOO2"
- key: "FOO3"
- toolLocation:
locations:
- home: "/home/user/bin/git"
key: "hudson.plugins.git.GitTool$DescriptorImpl@Default"
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package io.jenkins.plugins.casc.core;

import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.Extension;
import hudson.slaves.EnvironmentVariablesNodeProperty;
import hudson.slaves.EnvironmentVariablesNodeProperty.Entry;
import io.jenkins.plugins.casc.ConfigurationContext;
import io.jenkins.plugins.casc.ConfiguratorException;
import io.jenkins.plugins.casc.impl.configurators.DataBoundConfigurator;
import io.jenkins.plugins.casc.model.CNode;
import io.jenkins.plugins.casc.model.Mapping;
import java.util.ArrayList;
import java.util.List;

@Extension
public class GlobalNodePropertiesConfigurator extends DataBoundConfigurator<EnvironmentVariablesNodeProperty> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO this is the wrong fix and it is

if (value == null || value.length() == 0) {
return null;
}
which should be corrected. If a field is set to the empty string (rather than null), we should export

thatField: ''

regardless of where it is. If some config class neglects to call Util.fixEmpty (or fixEmptyAndTrim etc.) when processing a parameter to a @DataBoundSetter which cannot logically be the empty string, then fix that setter.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

that might have an huge impact on the export. Any single property with an empty String as value will be exported when nowadays is not

If some config class neglects to call Util.fixEmpty (or fixEmptyAndTrim etc.) when processing a parameter to a @DataBoundSetter which cannot logically be the empty string, then fix that setter.

which might mean to fix how many plugins? Although I agree with that being the 100% correct solution, that might be little realistic as users using the export nowadays will see unexpected changes in the export without a (from the user perspective) good reason

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

which might mean to fix how many plugins?

Not sure, but they need to be fixed anyway even without JCasC.


public GlobalNodePropertiesConfigurator() {
this(EnvironmentVariablesNodeProperty.class);
}

public GlobalNodePropertiesConfigurator(Class<?> clazz) {
super(EnvironmentVariablesNodeProperty.class);
}

@NonNull
@Override
public String getName() {
return "globalNodeProperties";
}

@NonNull
@Override
public EnvironmentVariablesNodeProperty configure(CNode c, ConfigurationContext context)
throws ConfiguratorException {
Mapping mapping = c.asMapping();
List<Entry> variables = getVarsAsList(mapping);
return new EnvironmentVariablesNodeProperty(variables);
}

private List<Entry> getVarsAsList(Mapping m) {
List<Entry> result = new ArrayList<>();
if (m.get("env") != null) {
result = m.get("env").asSequence().stream()
.map(pair -> {
if (pair.asMapping().get("key") == null) {
return null;
}
final String key =
pair.asMapping().get("key").asScalar().getValue();
final String value = pair.asMapping().get("value") == null
? ""
: pair.asMapping().get("value").asScalar().getValue();

return new Entry(key, value);
})
.toList();
}
return result;
}
}
Loading