Skip to content
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
@@ -0,0 +1,64 @@
package io.jenkins.plugins.casc;

import static io.jenkins.plugins.casc.misc.Util.getJenkinsRoot;
import static io.jenkins.plugins.casc.misc.Util.toStringFromYamlFile;
import static io.jenkins.plugins.casc.misc.Util.toYamlString;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.hasSize;

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.Env;
import io.jenkins.plugins.casc.misc.EnvVarsRule;
import io.jenkins.plugins.casc.misc.Envs;
import io.jenkins.plugins.casc.misc.JenkinsConfiguredWithCodeRule;
import io.jenkins.plugins.casc.model.CNode;
import java.util.Map;
import org.hamcrest.core.Is;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.RuleChain;

public class GlobalNodePropertiesWithEnvaVarsTest {

private JenkinsConfiguredWithCodeRule j = new JenkinsConfiguredWithCodeRule();

@Rule
public RuleChain chain = RuleChain.outerRule(new EnvVarsRule()).around(j);

@Test
@ConfiguredWithCode("GlobalNodePropertiesWithEnvVarsTest.yml")
@Envs({@Env(name = "VALUE_1", value = "BAR"), @Env(name = "TEST_GIT_HOME", value = "git-home")})
public void configureWithEnvVarsTest() {
DescribableList<NodeProperty<?>, NodePropertyDescriptor> nodeProperties = j.jenkins.getGlobalNodeProperties();
Map<String, String> envVars = ((EnvironmentVariablesNodeProperty)
nodeProperties.get(EnvironmentVariablesNodeProperty.class))
.getEnvVars();

assertThat(envVars.size(), is(2));
assertThat(envVars.get("FOO"), is("BAR"));
assertThat(envVars.get("FOO2"), is(""));

ToolLocationNodeProperty toolLocations = nodeProperties.get(ToolLocationNodeProperty.class);
assertThat(toolLocations.getLocations(), hasSize(1));
assertThat(toolLocations.getLocations().get(0).getHome(), is("git-home"));
}

@Test
@ConfiguredWithCode("GlobalNodePropertiesWithEnvVarsTest.yml")
@Envs({@Env(name = "VALUE_1", value = "BAR"), @Env(name = "TEST_GIT_HOME", value = "git-home")})
public void export() throws Exception {
ConfiguratorRegistry registry = ConfiguratorRegistry.get();
ConfigurationContext context = new ConfigurationContext(registry);
CNode yourAttribute = getJenkinsRoot(context).get("globalNodeProperties");

String exported = toYamlString(yourAttribute);
String expected = toStringFromYamlFile(this, "GlobalNodePropertiesWithEnvVarsTestExpected.yml");
assertThat(exported, Is.is(expected));
}
}
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
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.

This (FOO3) just looks like a user error to me ⇒ the bundle should be rejected.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

UI allows to save an empty env var (regardless why would someone want that), so theoretically configuration as code should allow creating that configuration too.

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.

I am talking about FOO3 (null) not FOO2 (empty).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's the point in allowing importing the exported yaml.
Empty vars will still be exported with no value, so it's either allowing keys without a value when importing or changing the export (which was discussed here).
I think we all agree that would be the optimal solution, but when testing that it impacted exports from several plugins, so that's not what we're trying to cover with this change

Copy link
Copy Markdown
Member

@timja timja Apr 10, 2025

Choose a reason for hiding this comment

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

the point is that

- key: FOO
   (no value)

is invalid and should not be allowed (unless it works right now and this is just testing something pre-existing)

a null env var value is invalid.
an empty string env var uncommon, but I guess

so ok:

- key: FOO
  value: ""

not ok:

- key: FOO

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.

Can we not just limit the behavioral change to this setting, leaving other misbehaving plugins for another day?

Copy link
Copy Markdown
Member

@timja timja Apr 14, 2025

Choose a reason for hiding this comment

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

Looks do-able from a quick look, something similar to this on CNode and then allow setting the value when you construct it.

Not sure how easy it would be to integrate when using databoundconfigurator though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm trying that, with no luck for the moment

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.

(my link was missing added now)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated be00ec1:

  • Removed the changes when configuring, so importing null vars is not supported
  • Added a false by default flag to CNode to allow printing when empty and setting it to trtue only for non null env vars

Will update PR description accordingly

- 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"
Comment thread
PereBueno marked this conversation as resolved.
Outdated
- 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,12 @@
jenkins:
globalNodeProperties:
- envVars:
env:
- key: FOO
value: ${VALUE_1}
- key: FOO2
value: ${VALUE_2}
- toolLocation:
locations:
- home: "${TEST_GIT_HOME}"
key: "hudson.plugins.git.GitTool$DescriptorImpl@Default"
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
- envVars:
env:
- key: "FOO"
value: "BAR"
- key: "FOO2"
Comment thread
PereBueno marked this conversation as resolved.
- toolLocation:
locations:
- home: "git-home"
key: "hudson.plugins.git.GitTool$DescriptorImpl@Default"
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
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> {

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, context);
return new EnvironmentVariablesNodeProperty(variables);
}

private List<Entry> getVarsAsList(Mapping m, ConfigurationContext context) {
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;
}
Comment thread
PereBueno marked this conversation as resolved.
Outdated
final String key =
pair.asMapping().get("key").asScalar().getValue();
final String value = pair.asMapping().get("value") == null
? ""
Comment thread
PereBueno marked this conversation as resolved.
Outdated
: context.getSecretSourceResolver()
.resolve(pair.asMapping()
.get("value")
.asScalar()
.getValue());

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