From fc362c35d4689f36323d7dc4e30d6746fe593578 Mon Sep 17 00:00:00 2001 From: Klaus Azesberger Date: Fri, 26 Apr 2013 15:09:57 +0200 Subject: [PATCH 1/3] global properties were overwritten by Environment Variables of the slave (which are outdated if you dont restart slave nodes after changing global properties) CAUTION - this might break some features: EnvironmentContributors don't get recognized any more (because CoreEnvironmentContributor did a Computer.current.getEnvironment() which also leads to overwriting with outdated values. --- pom.xml | 6 +++--- .../plugins/envinject/EnvInjectListener.java | 2 +- .../envinject/service/EnvInjectVariableGetter.java | 11 ++++++----- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/pom.xml b/pom.xml index ac7897f1..4894fa8e 100644 --- a/pom.xml +++ b/pom.xml @@ -6,7 +6,7 @@ org.jenkins-ci.plugins plugin - 1.444 + 1.480.3 envinject @@ -98,6 +98,6 @@ - - + + diff --git a/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectListener.java b/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectListener.java index c5942adb..548539b9 100644 --- a/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectListener.java +++ b/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectListener.java @@ -255,7 +255,7 @@ private Environment setUpEnvironmentWithoutJobPropertyObject(AbstractBuild build Map previousEnvVars = variableGetter.getEnvVarsPreviousSteps(build, logger); resultVariables.putAll(previousEnvVars); - resultVariables.putAll(variableGetter.getJenkinsSystemVariables(false)); + resultVariables.putAll(variableGetter.getJenkinsSystemVariables(true)); resultVariables.putAll(variableGetter.getBuildVariables(build, logger)); final FilePath rootPath = getNodeRootPath(); diff --git a/src/main/java/org/jenkinsci/plugins/envinject/service/EnvInjectVariableGetter.java b/src/main/java/org/jenkinsci/plugins/envinject/service/EnvInjectVariableGetter.java index 7503f6d7..0b83c5ae 100644 --- a/src/main/java/org/jenkinsci/plugins/envinject/service/EnvInjectVariableGetter.java +++ b/src/main/java/org/jenkinsci/plugins/envinject/service/EnvInjectVariableGetter.java @@ -66,11 +66,12 @@ public Map getBuildVariables(AbstractBuild build, EnvInjectLogge result.putAll(build.getCharacteristicEnvVars()); try { - EnvVars envVars = new EnvVars(); - for (EnvironmentContributor ec : EnvironmentContributor.all()) { - ec.buildEnvironmentFor(build, envVars, new LogTaskListener(LOG, Level.ALL)); - result.putAll(envVars); - } +// EnvVars envVars = new EnvVars(); +// for (EnvironmentContributor ec : EnvironmentContributor.all()) { +// ec.buildEnvironmentFor(build, envVars, new LogTaskListener(LOG, Level.ALL)); +// result.putAll(envVars); +// } + JDK jdk = build.getProject().getJDK(); if (jdk != null) { From 974172d4e26daeef4c7b3290af52d7cdf2ffbdc4 Mon Sep 17 00:00:00 2001 From: Klaus Azesberger Date: Wed, 8 May 2013 16:53:21 +0200 Subject: [PATCH 2/3] using 1.444 jenkins (minimum required jenkins version) --- pom.xml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pom.xml b/pom.xml index 4894fa8e..ac7897f1 100644 --- a/pom.xml +++ b/pom.xml @@ -6,7 +6,7 @@ org.jenkins-ci.plugins plugin - 1.480.3 + 1.444 envinject @@ -98,6 +98,6 @@ - - + + From de4acd15a19c1e2d0fb7b456b21013d948e4bf70 Mon Sep 17 00:00:00 2001 From: Klaus Azesberger Date: Wed, 15 May 2013 14:45:57 +0200 Subject: [PATCH 3/3] reworked: Change Of Global Property Gets Recognized When Without Job Property And Run On Slaves without overwriting "real" environment variables like PATH etc. wrote integration and unit test for this feature. added test-scoped dependency to powermock framework to be able to mock dependencies to static calls. --- pom.xml | 33 ++++++++- .../plugins/envinject/EnvInjectListener.java | 4 +- .../service/EnvInjectVariableGetter.java | 10 ++- .../envinject/GlobalPropertiesTest.java | 74 +++++++++++++++++-- .../sevice/EnvInjectVariableGetterTest.java | 66 +++++++++++++++-- 5 files changed, 167 insertions(+), 20 deletions(-) diff --git a/pom.xml b/pom.xml index ac7897f1..bc763996 100644 --- a/pom.xml +++ b/pom.xml @@ -1,5 +1,5 @@ + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd"> 4.0.0 @@ -43,6 +43,7 @@ 1.21 4.9 1.8.5 + 1.4.9 @@ -50,6 +51,23 @@ scm:git:git@github.com:jenkinsci/envinject-plugin.git + + + + org.apache.maven.plugins + maven-surefire-plugin + 2.14.1 + + + org.apache.maven.surefire + surefire-junit4 + 2.14.1 + + + + + + org.jenkins-ci.main @@ -82,6 +100,15 @@ ${mockito.version} test + + + org.powermock + powermock-mockito-release-full + ${powermock.version} + pom + test + + @@ -98,6 +125,6 @@ - - + + diff --git a/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectListener.java b/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectListener.java index 548539b9..d30833db 100644 --- a/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectListener.java +++ b/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectListener.java @@ -246,7 +246,7 @@ public void buildEnvVars(Map env) { }; } - private Environment setUpEnvironmentWithoutJobPropertyObject(AbstractBuild build, Launcher launcher, BuildListener listener) throws IOException, InterruptedException, EnvInjectException { + protected Environment setUpEnvironmentWithoutJobPropertyObject(AbstractBuild build, Launcher launcher, BuildListener listener) throws IOException, InterruptedException, EnvInjectException { final Map resultVariables = new HashMap(); @@ -255,7 +255,7 @@ private Environment setUpEnvironmentWithoutJobPropertyObject(AbstractBuild build Map previousEnvVars = variableGetter.getEnvVarsPreviousSteps(build, logger); resultVariables.putAll(previousEnvVars); - resultVariables.putAll(variableGetter.getJenkinsSystemVariables(true)); + resultVariables.putAll(variableGetter.getJenkinsSystemVariables(false)); resultVariables.putAll(variableGetter.getBuildVariables(build, logger)); final FilePath rootPath = getNodeRootPath(); diff --git a/src/main/java/org/jenkinsci/plugins/envinject/service/EnvInjectVariableGetter.java b/src/main/java/org/jenkinsci/plugins/envinject/service/EnvInjectVariableGetter.java index 0b83c5ae..4c973692 100644 --- a/src/main/java/org/jenkinsci/plugins/envinject/service/EnvInjectVariableGetter.java +++ b/src/main/java/org/jenkinsci/plugins/envinject/service/EnvInjectVariableGetter.java @@ -1,10 +1,9 @@ package org.jenkinsci.plugins.envinject.service; -import hudson.EnvVars; import hudson.Util; import hudson.matrix.MatrixRun; import hudson.model.*; -import hudson.util.LogTaskListener; +import hudson.slaves.EnvironmentVariablesNodeProperty; import org.jenkinsci.lib.envinject.EnvInjectException; import org.jenkinsci.lib.envinject.EnvInjectLogger; import org.jenkinsci.plugins.envinject.EnvInjectJobProperty; @@ -16,7 +15,6 @@ import java.util.List; import java.util.Map; import java.util.TreeMap; -import java.util.logging.Level; import java.util.logging.Logger; /** @@ -54,6 +52,12 @@ public Map getJenkinsSystemVariables(boolean forceOnMaster) thro result.put("JENKINS_HOME", Hudson.getInstance().getRootDir().getPath()); result.put("HUDSON_HOME", Hudson.getInstance().getRootDir().getPath()); // legacy compatibility + List globalNodeProperties = Hudson.getInstance().getGlobalNodeProperties() + .getAll(EnvironmentVariablesNodeProperty.class); + for (EnvironmentVariablesNodeProperty environmentVariablesNodeProperty : globalNodeProperties) { + result.putAll(environmentVariablesNodeProperty.getEnvVars()); + } + return result; } diff --git a/src/test/java/org/jenkinsci/plugins/envinject/GlobalPropertiesTest.java b/src/test/java/org/jenkinsci/plugins/envinject/GlobalPropertiesTest.java index ccd5c2ac..c37da515 100644 --- a/src/test/java/org/jenkinsci/plugins/envinject/GlobalPropertiesTest.java +++ b/src/test/java/org/jenkinsci/plugins/envinject/GlobalPropertiesTest.java @@ -1,17 +1,27 @@ package org.jenkinsci.plugins.envinject; import hudson.model.FreeStyleBuild; +import hudson.model.Result; +import hudson.model.Computer; import hudson.model.FreeStyleProject; import hudson.model.Hudson; -import hudson.model.Result; -import hudson.slaves.EnvironmentVariablesNodeProperty; +import hudson.model.Node; import hudson.slaves.NodeProperty; import hudson.slaves.NodePropertyDescriptor; +import hudson.slaves.EnvironmentVariablesNodeProperty; +import hudson.slaves.SlaveComputer; +import hudson.tasks.Shell; import hudson.util.DescribableList; -import junit.framework.Assert; -import org.jvnet.hudson.test.HudsonTestCase; import java.util.Map; +import java.util.Map.Entry; +import java.util.Set; + +import junit.framework.Assert; + +import org.apache.commons.lang.StringUtils; +import org.junit.Test; +import org.jvnet.hudson.test.HudsonTestCase; /** * @author Gregory Boissinot @@ -27,7 +37,7 @@ public void setUp() throws Exception { project = createFreeStyleProject(); } - + @Test public void testGlobalPropertiesWithWORKSPACE() throws Exception { final String testWorkspaceVariableName = "TEST_WORKSPACE"; @@ -59,6 +69,7 @@ public void testGlobalPropertiesWithWORKSPACE() throws Exception { } //Specific use case: We set a global workspace at job level + @Test public void testGlobalPropertiesSetWORKSPACE() throws Exception { final String testGlobalVariableName = "WORKSPACE"; @@ -90,4 +101,57 @@ public void testGlobalPropertiesSetWORKSPACE() throws Exception { assertEquals(result_testGlobalVariableName, result_testJobVariableName); } + + @Test + public void testChangeOfGlobalPropertyGetsRecognizedWhenWithoutJobPropertyAndRunOnSlaves() throws Exception { + + final String testVariableName = "TESTVAR"; + final String testVariableValue = "value1"; + final String testVariableValueAfterChange = "value2"; + + //A global node property TESTVAR + DescribableList, NodePropertyDescriptor> globalNodeProperties = Hudson.getInstance().getGlobalNodeProperties(); + EnvironmentVariablesNodeProperty.Entry testVarEntry = new EnvironmentVariablesNodeProperty.Entry(testVariableName, testVariableValue); + EnvironmentVariablesNodeProperty testVarNodePropertyItem = new EnvironmentVariablesNodeProperty(testVarEntry); + globalNodeProperties.add(testVarNodePropertyItem); + + Node slaveNode = createOnlineSlave(); + project.setAssignedNode(slaveNode); + project.getBuildersList().add(new Shell("echo \"TESTVAR=$TESTVAR\"")); + + // we do NOT add a jobProperty - we want to test "default" behaviour of jenkins with installed envinject plugin + // so we run the build right away + FreeStyleBuild build = project.scheduleBuild2(0).get(); + Assert.assertEquals(Result.SUCCESS, build.getResult()); + assertEquals(slaveNode.getNodeName(), build.getBuiltOn().getNodeName()); + + // assert correct injection of testVariable #1 + org.jenkinsci.lib.envinject.EnvInjectAction action = build.getAction(org.jenkinsci.lib.envinject.EnvInjectAction.class); + Map envVars = action.getEnvMap(); + String actualTestVariableValueInBuild = envVars.get(testVariableName); + assertNotNull("actual testVariableValue is null", actualTestVariableValueInBuild); + assertEquals(testVariableValue, actualTestVariableValueInBuild); + + Set> beforeChange = hudson.getComputer(slaveNode.getNodeName()).getEnvironment().entrySet(); + + // now we change the global property variable value... + testVarEntry = new EnvironmentVariablesNodeProperty.Entry(testVariableName, testVariableValueAfterChange); + Hudson.getInstance().getGlobalNodeProperties().add(new EnvironmentVariablesNodeProperty(testVarEntry)); + + Set> afterChange = hudson.getComputer(slaveNode.getNodeName()).getEnvironment().entrySet(); + // environment of the slave does not change without restarting it. assert it to make test fail if there will be + // some kind of auto-restart to reload config. + Assert.assertEquals(beforeChange, afterChange); + Assert.assertEquals(beforeChange.toString(), afterChange.toString()); + + //...run the job again... + FreeStyleBuild secondBuild = project.scheduleBuild2(0).get(); + Assert.assertEquals(Result.SUCCESS, secondBuild.getResult()); + assertEquals(slaveNode.getNodeName(), secondBuild.getBuiltOn().getNodeName()); + + //...and expect the testvariable to have the changed value + org.jenkinsci.lib.envinject.EnvInjectAction action2 = secondBuild.getAction(org.jenkinsci.lib.envinject.EnvInjectAction.class); + assertNotNull("actual testVariableValue is null", action2.getEnvMap().get(testVariableName)); + assertEquals(testVariableValueAfterChange, action2.getEnvMap().get(testVariableName)); + } } diff --git a/src/test/java/org/jenkinsci/plugins/envinject/sevice/EnvInjectVariableGetterTest.java b/src/test/java/org/jenkinsci/plugins/envinject/sevice/EnvInjectVariableGetterTest.java index df231a5a..866ea3af 100644 --- a/src/test/java/org/jenkinsci/plugins/envinject/sevice/EnvInjectVariableGetterTest.java +++ b/src/test/java/org/jenkinsci/plugins/envinject/sevice/EnvInjectVariableGetterTest.java @@ -1,23 +1,41 @@ package org.jenkinsci.plugins.envinject.sevice; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import hudson.EnvVars; import hudson.matrix.MatrixRun; import hudson.model.AbstractBuild; +import hudson.model.Computer; +import hudson.model.Hudson; +import hudson.model.Node; +import hudson.model.labels.LabelAtom; +import hudson.slaves.EnvironmentVariablesNodeProperty; +import hudson.slaves.NodeProperty; +import hudson.slaves.NodePropertyDescriptor; +import hudson.util.DescribableList; + +import java.io.File; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; + +import junit.framework.Assert; + import org.jenkinsci.lib.envinject.EnvInjectLogger; import org.jenkinsci.plugins.envinject.EnvInjectPluginAction; import org.jenkinsci.plugins.envinject.service.EnvInjectVariableGetter; import org.junit.Before; import org.junit.Test; - -import java.util.HashMap; -import java.util.Map; - -import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +import org.junit.runner.RunWith; +import org.powermock.api.mockito.PowerMockito; +import org.powermock.core.classloader.annotations.PrepareForTest; +import org.powermock.modules.junit4.PowerMockRunner; /** * @author Gregory Boissinot */ +@RunWith(PowerMockRunner.class) public class EnvInjectVariableGetterTest { private EnvInjectVariableGetter variableGetter; @@ -65,6 +83,40 @@ public void getEnvVarsPreviousStepsWithEnvInjectActionMatrixRun() throws Excepti assertTrue(sameMap(expectedEnvVars, resultEnvVars)); } + @Test + @PrepareForTest({ Computer.class, Hudson.class }) + public void testGetJenkinsSystemVariablesForceFetchesGlobalNodesPropertiesFromMaster() throws Exception { + + PowerMockito.mockStatic(Computer.class); + PowerMockito.mockStatic(Hudson.class); + Computer computer = mock(Computer.class); + Node node = mock(Node.class); + EnvVars envVars = new EnvVars(); + final String PROPERTY_KEY = "PATH"; + final String VALUE_FROM_SLAVE_COMPUTER = "VALUE_FROM_SLAVE_COMPUTER"; + final String VALUE_FROM_GNP_MASTER = "VALUE_FROM_GNP_MASTER"; + envVars.put(PROPERTY_KEY, VALUE_FROM_SLAVE_COMPUTER); + Hudson hudson = mock(Hudson.class); + + when(Computer.currentComputer()).thenReturn(computer); + when(computer.getNode()).thenReturn(node); + when(computer.getEnvironment()).thenReturn(envVars); + when(node.getAssignedLabels()).thenReturn(new HashSet()); + when(computer.getName()).thenReturn("slave0"); + when(Hudson.getInstance()).thenReturn(hudson); + when(hudson.getRootDir()).thenReturn(new File("")); + + DescribableList, NodePropertyDescriptor> globalNodeProperties = new DescribableList, NodePropertyDescriptor>( + hudson); + EnvironmentVariablesNodeProperty property = new EnvironmentVariablesNodeProperty( + new EnvironmentVariablesNodeProperty.Entry(PROPERTY_KEY, VALUE_FROM_GNP_MASTER)); + globalNodeProperties.add(property); + when(Hudson.getInstance().getGlobalNodeProperties()).thenReturn(globalNodeProperties); + + Map jenkinsSystemVariables = variableGetter.getJenkinsSystemVariables(false); + Assert.assertNotNull(jenkinsSystemVariables); + Assert.assertEquals(VALUE_FROM_GNP_MASTER, jenkinsSystemVariables.get(PROPERTY_KEY)); + } private boolean sameMap(Map expectedMap, Map actualMap) {