Skip to content
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.

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

Added executionType parameter to have an ability to run phase jobs SE… #929

Merged
merged 3 commits into from
Oct 10, 2016

Conversation

sanicheev
Copy link

I've added an ability to switch between execution types in multi jobs step plugin.
This options was missed in job dsl plugin.
Basically it can switch between sequential or parallel execution type of jobs within the phase.

*/
void phase(String name, String continuationCondition, @DslContext(PhaseContext) Closure phaseClosure) {
PhaseContext phaseContext = new PhaseContext(jobManagement, item, name, continuationCondition)
void phase(
Copy link
Member

Choose a reason for hiding this comment

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

This will break compatibility as it changes a signature. Leave the signature as it is, just adding a new method to PhaseContext is sufficient.

PhaseContext(JobManagement jobManagement, Item item, String phaseName, String continuationCondition) {
PhaseContext(
JobManagement jobManagement, Item item, String phaseName, String continuationCondition,
String executionType
Copy link
Member

Choose a reason for hiding this comment

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

Don't add another constructor parameter here. Set the field to the default value.

@@ -36,6 +41,13 @@ class PhaseContext extends AbstractContext {
}

/**
* Defines how to run the whole MultiJob phase.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Add a @since tag which points to the next version.

ContextHelper.executeInContext(phaseClosure, phaseContext)

Preconditions.checkNotNullOrEmpty(phaseContext.phaseName, 'A phase needs a name')
Preconditions.checkArgument(
VALID_CONTINUATION_CONDITIONS.contains(phaseContext.continuationCondition),
"Continuation Condition needs to be one of these values: ${VALID_CONTINUATION_CONDITIONS.join(', ')}"
)
Preconditions.checkArgument(
Copy link
Member

Choose a reason for hiding this comment

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

Move this check to the executionType method in PhaseContext.

@@ -36,6 +41,13 @@ class PhaseContext extends AbstractContext {
}

/**
* Defines how to run the whole MultiJob phase.
*/
void executionType(String executionType) {
Copy link
Member

Choose a reason for hiding this comment

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

This feature need version 1.22 of the MultiJob plugin. Add a @RequiresPlugin annotation.

@sanicheev
Copy link
Author

Suggested fixes has been applied

Copy link
Member

@daspilker daspilker left a comment

Choose a reason for hiding this comment

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

This is going in the right direction, there are a few things that I forgot to mention last time.

@@ -49,6 +49,7 @@ class MultiJobStepContext extends StepContext {
stepNodes << new NodeBuilder().'com.tikal.jenkins.plugins.multijob.MultiJobBuilder' {
phaseName phaseContext.phaseName
delegate.continuationCondition(phaseContext.continuationCondition)
delegate.executionType(phaseContext.executionType)
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be wrapped in an if block testing jobManagement.isMinimumPluginVersionInstalled(...) so that the new element only gets generated if the minimum required plugin version is installed.

thrown(DslScriptException)

where:
execution << ['FOO']
Copy link
Member

Choose a reason for hiding this comment

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

do not use a where section if there is only one option

}

where:
execution << ['SEQUENTIALLY']
Copy link
Member

Choose a reason for hiding this comment

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

do not use a where section if there is only one option, but you could also test both supported options here

Copy link
Author

@sanicheev sanicheev Oct 10, 2016

Choose a reason for hiding this comment

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

I think this is a good option to modify codenarcTest to catch such conditions

@sanicheev
Copy link
Author

Suggested fixes has been applied

@daspilker daspilker merged commit f37a813 into jenkinsci:master Oct 10, 2016
@sanicheev sanicheev deleted the executionType branch October 27, 2016 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants