From e19af3db5a47aa479ba8c91bc1fddf8b71627c98 Mon Sep 17 00:00:00 2001 From: Martin Nonnenmacher Date: Wed, 7 Feb 2018 10:54:41 +0100 Subject: [PATCH] Fix #65: Do not add common DSL closure multiple times in PipelineBuilder Pass the common DSL closure to the build method of the job builder instead of adding it to the list of dsl closures. This prevents it from being added multiple times when PipelineBuilder.build() is called multiple times. Add byte-buddy to the test dependencies to enable Spock to mock PipelineJobBuilder which is not an interface [1][2]. [1] http://spockframework.org/spock/docs/1.1/all_in_one.html#_mocking_classes [2] http://spockframework.org/spock/docs/1.1/all_in_one.html#_1_1_rc_3_released_2016_10_17 --- CHANGELOG.md | 1 + plugin/build.gradle | 2 ++ .../plugins/jobdsl/util/JobBuilder.groovy | 30 ++++++++++++------- .../jobdsl/util/PipelineBuilder.groovy | 7 +---- .../jobdsl/util/PipelineBuilderSpec.groovy | 13 ++++---- 5 files changed, 31 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e7b6db..73f9607 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). - Upgrade job-dsl-core to 1.67. - Process Job DSL scripts in specific order. Scripts from the same folder are processed in alphabetical order and before any scripts from subfolders. Subfolders are also processed in alphabetical order. +- Fix #65 - PipelineBuilder: Add common DSL Closure only once when PipelineBuilder.build() is called multiple times. ## 3.0.0 (2018-01-10) diff --git a/plugin/build.gradle b/plugin/build.gradle index 82f541f..886ba0d 100644 --- a/plugin/build.gradle +++ b/plugin/build.gradle @@ -80,6 +80,8 @@ dependencies { exclude(module: 'groovy-all') } + testCompile 'net.bytebuddy:byte-buddy:1.7.9' + functionalTestCompile('org.spockframework:spock-core:1.1-groovy-2.4') { exclude(module: 'groovy-all') } diff --git a/plugin/src/main/groovy/com/here/gradle/plugins/jobdsl/util/JobBuilder.groovy b/plugin/src/main/groovy/com/here/gradle/plugins/jobdsl/util/JobBuilder.groovy index 0a01026..3462285 100644 --- a/plugin/src/main/groovy/com/here/gradle/plugins/jobdsl/util/JobBuilder.groovy +++ b/plugin/src/main/groovy/com/here/gradle/plugins/jobdsl/util/JobBuilder.groovy @@ -31,24 +31,26 @@ class JobBuilder { /** * Create the job object using the configured DSL closures and job class. * + * @param dslClosure An optional Job DSL Closure that will be applied to the job builder. + * * @return The created {@link Job} object. */ - final Job build() { + final Job build(Closure dslClosure = { }) { checkNameIsValid() - def dslClosure = concatenateDslClosures() + def combinedClosure = concatenateDslClosures(dslClosure) switch (jobClass) { case BuildFlowJob: - return dslFactory.buildFlowJob(fullJobName(), dslClosure) + return dslFactory.buildFlowJob(fullJobName(), combinedClosure) case FreeStyleJob: - return dslFactory.freeStyleJob(fullJobName(), dslClosure) + return dslFactory.freeStyleJob(fullJobName(), combinedClosure) case MatrixJob: - return dslFactory.matrixJob(fullJobName(), dslClosure) + return dslFactory.matrixJob(fullJobName(), combinedClosure) case MavenJob: - return dslFactory.mavenJob(fullJobName(), dslClosure) + return dslFactory.mavenJob(fullJobName(), combinedClosure) case MultiJob: - return dslFactory.multiJob(fullJobName(), dslClosure) + return dslFactory.multiJob(fullJobName(), combinedClosure) case WorkflowJob: - return dslFactory.pipelineJob(fullJobName(), dslClosure) + return dslFactory.pipelineJob(fullJobName(), combinedClosure) default: throw new GradleJobDslPluginException("Job type ${jobClass} is not supported.") } @@ -201,8 +203,16 @@ class JobBuilder { } } - Closure concatenateDslClosures() { - return dslClosures.inject({ }) { acc, val -> acc >> val } + /** + * Concatenate all {@link #dslClosures} to a single Closure. + * + * @param dslClosure An optional DSL Closure that will be appended to the result. + * + * @return The concatenated closure. + */ + Closure concatenateDslClosures(Closure dslClosure = { }) { + def allClosures = [*dslClosures, dslClosure] + return allClosures.inject({ }) { acc, val -> acc >> val } } /** diff --git a/plugin/src/main/groovy/com/here/gradle/plugins/jobdsl/util/PipelineBuilder.groovy b/plugin/src/main/groovy/com/here/gradle/plugins/jobdsl/util/PipelineBuilder.groovy index 3ebe945..50b73e2 100644 --- a/plugin/src/main/groovy/com/here/gradle/plugins/jobdsl/util/PipelineBuilder.groovy +++ b/plugin/src/main/groovy/com/here/gradle/plugins/jobdsl/util/PipelineBuilder.groovy @@ -50,8 +50,7 @@ class PipelineBuilder { void build() { jobBuilders.each { jobBuilder -> applyDefaultConfiguration(jobBuilder) - applyCommonDsl(jobBuilder) - jobBuilder.build() + jobBuilder.build(commonDsl) } } @@ -65,8 +64,4 @@ class PipelineBuilder { } } - private applyCommonDsl(PipelineJobBuilder jobBuilder) { - jobBuilder.addDsl(commonDsl) - } - } diff --git a/plugin/src/test/groovy/com/here/gradle/plugins/jobdsl/util/PipelineBuilderSpec.groovy b/plugin/src/test/groovy/com/here/gradle/plugins/jobdsl/util/PipelineBuilderSpec.groovy index 7ff681c..b281516 100644 --- a/plugin/src/test/groovy/com/here/gradle/plugins/jobdsl/util/PipelineBuilderSpec.groovy +++ b/plugin/src/test/groovy/com/here/gradle/plugins/jobdsl/util/PipelineBuilderSpec.groovy @@ -1,5 +1,6 @@ package com.here.gradle.plugins.jobdsl.util +import com.here.gradle.plugins.jobdsl.GradleJobDslPluginException import javaposse.jobdsl.dsl.DslFactory import org.codehaus.groovy.runtime.ComposedClosure import spock.lang.Specification @@ -61,11 +62,7 @@ class PipelineBuilderSpec extends Specification { def 'common DSL is applied'() { given: - def dslFactory = Mock(DslFactory) - - def job = new PipelineJobBuilder(dslFactory) - job.name = 'job' - job.freeStyleJob { } + def job = Mock(PipelineJobBuilder) def pipelineBuilder = new PipelineBuilder() @@ -77,7 +74,11 @@ class PipelineBuilderSpec extends Specification { pipelineBuilder.build() then: - job.dslClosures.contains(commonDsl) + 1 * job.invokeMethod('concatenateDslClosures', [commonDsl]) + + // There is no easy way to set the jobClass property of the mocked job, so expect the job building to fail. + def ex = thrown GradleJobDslPluginException + ex.message == 'Job type null is not supported.' } def 'base folders are applied'() {