Skip to content

Allowing empty valued global variables in config file #2657

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

Merged
timja merged 5 commits into from
Apr 3, 2025
Merged

Allowing empty valued global variables in config file #2657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
695761b
Using configurator for global props
PereBueno Apr 1, 2025
3f5d776
Accept EnvVars wihtout value
fcojfernandez Apr 2, 2025
be60c5a
Testing additional global props
PereBueno Apr 2, 2025
e7055ab
Removing not used describe
PereBueno Apr 2, 2025
d1c08d9
Switching to use DataBoundConstructor
PereBueno Apr 3, 2025
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
33 changes: 30 additions & 3 deletions integrations/src/test/java/io/jenkins/plugins/casc/GlobalNodePropertiesTest.java
View file
Open in desktop
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
12 changes: 12 additions & 0 deletions integrations/src/test/resources/io/jenkins/plugins/casc/GlobalNodePropertiesTest.yml
View file
Open in desktop
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"
11 changes: 11 additions & 0 deletions integrations/src/test/resources/io/jenkins/plugins/casc/GlobalNodePropertiesTestExpected.yml
View file
Open in desktop
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"
61 changes: 61 additions & 0 deletions plugin/src/main/java/io/jenkins/plugins/casc/core/GlobalNodePropertiesConfigurator.java
View file
Open in desktop
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

@jglick jglick Apr 7, 2025

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

configuration-as-code-plugin/plugin/src/main/java/io/jenkins/plugins/casc/ConfigurationAsCode.java

Lines 660 to 662 in 148f87d

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

@fcojfernandez fcojfernandez Apr 8, 2025

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

@jglick jglick Apr 9, 2025

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