From b6fd507145bfba230c4a9d690e62a8eb34ead0f3 Mon Sep 17 00:00:00 2001 From: Marcus Holl Date: Fri, 31 Aug 2018 15:41:31 +0200 Subject: [PATCH 1/6] Be more precise with libraryResource and read yaml in prepareDefaultValuesTest In the free wild it is the duty of libraryResource to provide some resource as text and it is the duty of readYaml to parse a text (... would also be possible to read a file instead just parsing text, but that is a different story). In our setup we just forwarded the resource name in libraryResource and reacted on that resource name inside read yaml. There was no yaml parsing at all, the yaml 'as it' was returned. The approach now is much closer to reality. Library resource now provides the text 'behind' the resource and yaml parses it. There is now a real yaml parsing. Read yaml is now not registered explicitly anymore. It is just the readYaml closure which is registerd by default for our tests. --- test/groovy/PrepareDefaultValuesTest.groovy | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/test/groovy/PrepareDefaultValuesTest.groovy b/test/groovy/PrepareDefaultValuesTest.groovy index 0ae67d65e..8160d5382 100644 --- a/test/groovy/PrepareDefaultValuesTest.groovy +++ b/test/groovy/PrepareDefaultValuesTest.groovy @@ -28,16 +28,14 @@ public class PrepareDefaultValuesTest extends BasePiperTest { @Before public void setup() { - helper.registerAllowedMethod("libraryResource", [String], { fileName-> return fileName }) - helper.registerAllowedMethod("readYaml", [Map], { m -> - switch(m.text) { - case 'default_pipeline_environment.yml': return [default: 'config'] - case 'custom.yml': return [custom: 'myConfig'] + helper.registerAllowedMethod("libraryResource", [String], { fileName -> + switch(fileName) { + case 'default_pipeline_environment.yml': return "default: 'config'" + case 'custom.yml': return "custom: 'myConfig'" case 'not_found': throw new hudson.AbortException('No such library resource not_found could be found') - default: return [the:'end'] + default: return "the:'end'" } }) - } @Test From c54b6e6cf27b8c4ed16e4c288638f785d9fe48c0 Mon Sep 17 00:00:00 2001 From: Marcus Holl Date: Mon, 3 Sep 2018 09:38:41 +0200 Subject: [PATCH 2/6] Make DefaultConfigurationCache instance checks more precise up to now we check some values provided by the DefaultConfigurationCache. This is some kind of plausibility check, but this is not really a check that the instance did not change. With the tests as they are now we check in fact for a new instance. --- test/groovy/PrepareDefaultValuesTest.groovy | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/groovy/PrepareDefaultValuesTest.groovy b/test/groovy/PrepareDefaultValuesTest.groovy index 0ae67d65e..aba9fd3b8 100644 --- a/test/groovy/PrepareDefaultValuesTest.groovy +++ b/test/groovy/PrepareDefaultValuesTest.groovy @@ -52,11 +52,16 @@ public class PrepareDefaultValuesTest extends BasePiperTest { @Test public void testReInitializeOnCustomConfig() { - DefaultValueCache.createInstance([key:'value']) + def instance = DefaultValueCache.createInstance([key:'value']) // existing instance is dropped in case a custom config is provided. jsr.step.call(script: nullScript, customDefaults: 'custom.yml') + // this check is for checking we have another instance + assert ! instance.is(DefaultValueCache.getInstance()) + + // some additional checks that the configuration represented by the new + // config is fine assert DefaultValueCache.getInstance().getDefaultValues().size() == 2 assert DefaultValueCache.getInstance().getDefaultValues().default == 'config' assert DefaultValueCache.getInstance().getDefaultValues().custom == 'myConfig' @@ -65,10 +70,11 @@ public class PrepareDefaultValuesTest extends BasePiperTest { @Test public void testNoReInitializeWithoutCustomConfig() { - DefaultValueCache.createInstance([key:'value']) + def instance = DefaultValueCache.createInstance([key:'value']) jsr.step.call(script: nullScript) + assert instance.is(DefaultValueCache.getInstance()) assert DefaultValueCache.getInstance().getDefaultValues().size() == 1 assert DefaultValueCache.getInstance().getDefaultValues().key == 'value' } From 5cca5fddbb1989d403ff63b55c1135416a99a891 Mon Sep 17 00:00:00 2001 From: Marcus Holl Date: Mon, 3 Sep 2018 11:09:09 +0200 Subject: [PATCH 3/6] Tests: make use of JenkinsCredentialsRule JenkinsCredentialsRule now closer to reality since it mimics the bevavior or the credentials plugin in case a credential is not known. --- test/groovy/CloudFoundryDeployTest.groovy | 22 ++------------- test/groovy/NeoDeployTest.groovy | 27 +++++-------------- .../groovy/util/JenkinsCredentialsRule.groovy | 12 ++++++--- 3 files changed, 17 insertions(+), 44 deletions(-) diff --git a/test/groovy/CloudFoundryDeployTest.groovy b/test/groovy/CloudFoundryDeployTest.groovy index a5e5c41af..fd2865507 100644 --- a/test/groovy/CloudFoundryDeployTest.groovy +++ b/test/groovy/CloudFoundryDeployTest.groovy @@ -6,6 +6,7 @@ import org.junit.rules.ExpectedException import org.junit.rules.RuleChain import org.yaml.snakeyaml.Yaml import util.BasePiperTest +import util.JenkinsCredentialsRule import util.JenkinsEnvironmentRule import util.JenkinsDockerExecuteRule import util.JenkinsLoggingRule @@ -39,28 +40,9 @@ class CloudFoundryDeployTest extends BasePiperTest { .around(jwfr) .around(jedr) .around(jer) + .around(new JenkinsCredentialsRule(this).withCredentials('test_cfCredentialsId', 'test_cf', '********')) .around(jsr) // needs to be activated after jedr, otherwise executeDocker is not mocked - @Before - void init() throws Throwable { - helper.registerAllowedMethod('usernamePassword', [Map], { m -> return m }) - helper.registerAllowedMethod('withCredentials', [List, Closure], { l, c -> - if(l[0].credentialsId == 'test_cfCredentialsId') { - binding.setProperty('username', 'test_cf') - binding.setProperty('password', '********') - } else if(l[0].credentialsId == 'test_camCredentialsId') { - binding.setProperty('username', 'test_cam') - binding.setProperty('password', '********') - } - try { - c() - } finally { - binding.setProperty('username', null) - binding.setProperty('password', null) - } - }) - } - @Test void testNoTool() throws Exception { nullScript.commonPipelineEnvironment.configuration = [ diff --git a/test/groovy/NeoDeployTest.groovy b/test/groovy/NeoDeployTest.groovy index f18f0f866..045ebc52e 100644 --- a/test/groovy/NeoDeployTest.groovy +++ b/test/groovy/NeoDeployTest.groovy @@ -5,8 +5,10 @@ import org.junit.rules.TemporaryFolder import org.junit.BeforeClass import org.junit.ClassRule import org.junit.Ignore + import org.hamcrest.BaseMatcher import org.hamcrest.Description +import org.jenkinsci.plugins.credentialsbinding.impl.CredentialNotFoundException import org.junit.Assert import org.junit.Before import org.junit.Rule @@ -14,6 +16,7 @@ import org.junit.Test import org.junit.rules.ExpectedException import org.junit.rules.RuleChain import util.BasePiperTest +import util.JenkinsCredentialsRule import util.JenkinsLoggingRule import util.JenkinsShellCallRule import util.JenkinsStepRule @@ -37,6 +40,9 @@ class NeoDeployTest extends BasePiperTest { .around(thrown) .around(jlr) .around(jscr) + .around(new JenkinsCredentialsRule(this) + .withCredentials('myCredentialsId', 'anonymous', '********') + .withCredentials('CI_CREDENTIALS_ID', 'defaultUser', '********')) .around(jsr) private static workspacePath @@ -63,24 +69,6 @@ class NeoDeployTest extends BasePiperTest { helper.registerAllowedMethod('dockerExecute', [Map, Closure], null) helper.registerAllowedMethod('fileExists', [String], { s -> return new File(workspacePath, s).exists() }) - helper.registerAllowedMethod('usernamePassword', [Map], { m -> return m }) - helper.registerAllowedMethod('withCredentials', [List, Closure], { l, c -> - if(l[0].credentialsId == 'myCredentialsId') { - binding.setProperty('username', 'anonymous') - binding.setProperty('password', '********') - } else if(l[0].credentialsId == 'CI_CREDENTIALS_ID') { - binding.setProperty('username', 'defaultUser') - binding.setProperty('password', '********') - } - try { - c() - } finally { - binding.setProperty('username', null) - binding.setProperty('password', null) - } - - }) - helper.registerAllowedMethod('sh', [Map], { Map m -> getVersionWithEnvVars(m) }) nullScript.commonPipelineEnvironment.configuration = [steps:[neoDeploy: [host: 'test.deploy.host.com', account: 'trialuser123']]] @@ -176,8 +164,7 @@ class NeoDeployTest extends BasePiperTest { @Test void badCredentialsIdTest() { - thrown.expect(MissingPropertyException) - thrown.expectMessage('No such property: username') + thrown.expect(CredentialNotFoundException) jsr.step.call(script: nullScript, archivePath: archiveName, diff --git a/test/groovy/util/JenkinsCredentialsRule.groovy b/test/groovy/util/JenkinsCredentialsRule.groovy index 63133c372..5f6e7dd8c 100644 --- a/test/groovy/util/JenkinsCredentialsRule.groovy +++ b/test/groovy/util/JenkinsCredentialsRule.groovy @@ -14,6 +14,7 @@ import static org.hamcrest.Matchers.nullValue import static org.junit.Assert.assertThat; import org.hamcrest.Matchers +import org.jenkinsci.plugins.credentialsbinding.impl.CredentialNotFoundException /** * By default a user "anonymous" with password "********" @@ -47,16 +48,19 @@ class JenkinsCredentialsRule implements TestRule { @Override void evaluate() throws Throwable { - testInstance.helper.registerAllowedMethod('usernamePassword', [Map.class], {m -> return m}) + testInstance.helper.registerAllowedMethod('usernamePassword', [Map.class], + { m -> if (credentials.keySet().contains(m.credentialsId)) return m; + // this is what really happens in case of an unknown credentials id, + // checked with reality using credentials plugin 2.1.18. + throw new CredentialNotFoundException( + "Could not find credentials entry with ID '${m.credentialsId}'") + }) testInstance.helper.registerAllowedMethod('withCredentials', [List, Closure], { l, c -> def credsId = l[0].credentialsId def creds = credentials.get(credsId) - assertThat("Unexpected credentialsId received: '${credsId}'.", - creds, is(not(nullValue()))) - binding.setProperty('username', creds?.user) binding.setProperty('password', creds?.passwd) try { From b65eca35b232e4649c8341219c5b96727dd976b5 Mon Sep 17 00:00:00 2001 From: Marcus Holl Date: Thu, 6 Sep 2018 10:32:58 +0200 Subject: [PATCH 4/6] MapUtils: Provide method for traversing of nested maps. --- src/com/sap/piper/MapUtils.groovy | 23 +++++++++++++++++++ test/groovy/com/sap/piper/MapUtilsTest.groovy | 7 ++++++ 2 files changed, 30 insertions(+) diff --git a/src/com/sap/piper/MapUtils.groovy b/src/com/sap/piper/MapUtils.groovy index de4ad2096..356b07908 100644 --- a/src/com/sap/piper/MapUtils.groovy +++ b/src/com/sap/piper/MapUtils.groovy @@ -38,4 +38,27 @@ class MapUtils implements Serializable { return result } + + /** + * @param m The map to which the changed denoted by closure strategy + * should be applied. + * The strategy is also applied to all sub-maps contained as values + * in m in a recursive manner. + * @param strategy Strategy applied to all non-map entries + */ + @NonCPS + static void traverse(Map m, Closure strategy) { + + def updates = [:] + for(def e : m.entrySet()) { + if(isMap(e.value)) { + traverse(e.getValue(), strategy) + } + else + // do not update the map while it is traversed. Depending + // on the map implementation the behavior is undefined. + updates.put(e.key, strategy(e.value)) + } + m.putAll(updates) + } } diff --git a/test/groovy/com/sap/piper/MapUtilsTest.groovy b/test/groovy/com/sap/piper/MapUtilsTest.groovy index 2d38e9f0f..61a06aaf8 100644 --- a/test/groovy/com/sap/piper/MapUtilsTest.groovy +++ b/test/groovy/com/sap/piper/MapUtilsTest.groovy @@ -43,4 +43,11 @@ class MapUtilsTest { c: [ d: 'abc', e: '']] } + + @Test + void testTraverse() { + Map m = [a: 'x1', m:[b: 'x2', c: 'otherString']] + MapUtils.traverse(m, { s -> (s.startsWith('x')) ? "replaced" : s}) + assert m == [a: 'replaced', m: [b: 'replaced', c: 'otherString']] + } } From 685756f09d22cf34be7eaef3a06aea6c3e55ac13 Mon Sep 17 00:00:00 2001 From: Marcus Holl Date: Thu, 6 Sep 2018 11:13:08 +0200 Subject: [PATCH 5/6] Invoke method 'use' before returning configuration values In order to ensure all actions applied by 'use()' are also applied before returning a single configuration values. Possible optimization: introduce a flag indicating that 'use' has been called (not implemented for now). --- src/com/sap/piper/ConfigurationHelper.groovy | 1 + 1 file changed, 1 insertion(+) diff --git a/src/com/sap/piper/ConfigurationHelper.groovy b/src/com/sap/piper/ConfigurationHelper.groovy index b8e61d2d0..54f602460 100644 --- a/src/com/sap/piper/ConfigurationHelper.groovy +++ b/src/com/sap/piper/ConfigurationHelper.groovy @@ -103,6 +103,7 @@ class ConfigurationHelper implements Serializable { } def getConfigProperty(key) { + use() return getConfigPropertyNested(config, key) } From 55a952522b25ff4fa5c44aa19b7efacfa10c6563 Mon Sep 17 00:00:00 2001 From: Marcus Holl Date: Thu, 6 Sep 2018 11:16:46 +0200 Subject: [PATCH 6/6] ConfigurationHelper: replace all occurences of GString by java.lang.String GStrings might be handed over e.g. via signature to steps. GStrings in configuration makes it harder to deal with configurations. E.g. ```if(param == 'a' || param == 'b')``` returns true if a is a GString representing 'a' but ```if(param in ['a', 'b'])``` returns false. It would be possible not to use the ```in``` notation in our code. But this increases readability. GString are not significant and can be replaced by the corresponding java.lang.String representation without loss of information. Hence it is justified IMO to ensure there are no GStrings contained in the configuration map. --- src/com/sap/piper/ConfigurationHelper.groovy | 9 +++++- src/com/sap/piper/MapUtils.groovy | 5 ++-- .../sap/piper/ConfigurationHelperTest.groovy | 28 +++++++++++++++++++ 3 files changed, 39 insertions(+), 3 deletions(-) diff --git a/src/com/sap/piper/ConfigurationHelper.groovy b/src/com/sap/piper/ConfigurationHelper.groovy index 54f602460..88ee6af1d 100644 --- a/src/com/sap/piper/ConfigurationHelper.groovy +++ b/src/com/sap/piper/ConfigurationHelper.groovy @@ -1,5 +1,7 @@ package com.sap.piper +import com.cloudbees.groovy.cps.NonCPS + class ConfigurationHelper implements Serializable { static ConfigurationHelper loadStepDefaults(Script step){ return new ConfigurationHelper(step) @@ -96,7 +98,12 @@ class ConfigurationHelper implements Serializable { return this } - Map use(){ return config } + @NonCPS // required because we have a closure in the + // method body that cannot be CPS transformed + Map use(){ + MapUtils.traverse(config, { v -> (v instanceof GString) ? v.toString() : v }) + return config + } ConfigurationHelper(Map config = [:]){ this.config = config diff --git a/src/com/sap/piper/MapUtils.groovy b/src/com/sap/piper/MapUtils.groovy index 356b07908..784a281e7 100644 --- a/src/com/sap/piper/MapUtils.groovy +++ b/src/com/sap/piper/MapUtils.groovy @@ -54,10 +54,11 @@ class MapUtils implements Serializable { if(isMap(e.value)) { traverse(e.getValue(), strategy) } - else + else { // do not update the map while it is traversed. Depending // on the map implementation the behavior is undefined. - updates.put(e.key, strategy(e.value)) + updates.put(e.getKey(), strategy(e.getValue())) + } } m.putAll(updates) } diff --git a/test/groovy/com/sap/piper/ConfigurationHelperTest.groovy b/test/groovy/com/sap/piper/ConfigurationHelperTest.groovy index 4d82d55b0..634979743 100644 --- a/test/groovy/com/sap/piper/ConfigurationHelperTest.groovy +++ b/test/groovy/com/sap/piper/ConfigurationHelperTest.groovy @@ -279,4 +279,32 @@ class ConfigurationHelperTest { Assert.assertThat(configuration, hasEntry('collectTelemetryData', false)) } + + @Test + public void testGStringsAreReplacedByJavaLangStrings() { + // + // needed in order to ensure we have real GStrings. + // a GString not containing variables might be optimized to + // a java.lang.String from the very beginning. + def dummy = 'Dummy', + aGString = "a${dummy}", + bGString = "b${dummy}", + cGString = "c${dummy}" + + assert aGString instanceof GString + assert bGString instanceof GString + assert cGString instanceof GString + + def config = new ConfigurationHelper([a: aGString, + nextLevel: [b: bGString]]) + .mixin([c : cGString]) + .use() + + assert config == [a: 'aDummy', + c: 'cDummy', + nextLevel: [b: 'bDummy']] + assert config.a instanceof java.lang.String + assert config.c instanceof java.lang.String + assert config.nextLevel.b instanceof java.lang.String + } }