Skip to content

Commit

Permalink
[FIXED JENKINS-37828] Properly reject parallel.
Browse files Browse the repository at this point in the history 
More notably, reject it when it's not the only step in a block. In the
JSON representation, we don't even use the term parallel - we just
have multiple branches. In the groovy parsing, we look for parallel as
the only step in a stage and treat it differently (resulting in
multiple branches). So any time we see parallel as a step by the time
we get to validation, well, it's wrong. So, yeah.
  • Loading branch information
@abayer
abayer committed Sep 1, 2016
1 parent 1ddd0ff commit cb86d2b
Show file tree
Hide file tree
Showing 4 changed files with 102 additions and 53 deletions.
6 changes: 5 additions & 1 deletion src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTStep.groovy
View file
Expand Up @@ -17,7 +17,11 @@ public class ModelASTStep extends ModelASTElement {
/**
* A list of step names which are banned from being executed within a step block.
*/
public final List<String> blockedSteps = ["stage", "properties"]
public final Map<String, String> blockedSteps = [
"stage": "The stage step cannot be used in step blocks in Pipeline Config",
"properties": "The properties step cannot be used in step blocks in Pipeline Config",
"parallel": "The parallel step can only be used as the only top-level step in a stage's step block"
]

String name;
ModelASTArgumentList args;
Expand Down
103 changes: 52 additions & 51 deletions ...ain/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidator.groovy
View file
Expand Up @@ -184,73 +184,74 @@ class ModelValidator {
public boolean validateElement(@Nonnull ModelASTStep step) {
boolean valid = true

if (step.blockedSteps.contains(step.name)) {
errorCollector.error(step, "Invalid step '${step.name}' used - not allowed in this context")
if (step.blockedSteps.keySet().contains(step.name)) {
errorCollector.error(step, "Invalid step '${step.name}' used - not allowed in this context - ${step.blockedSteps.get(step.name)}")
valid = false
}

// We can't do step validation without a Jenkins instance, so move on.
if (Jenkins.getInstance() != null) {
Descriptor desc = lookupStepDescriptor(step.name)
DescribableModel<? extends Describable> model
} else {
// We can't do step validation without a Jenkins instance, so move on.
if (Jenkins.getInstance() != null) {
Descriptor desc = lookupStepDescriptor(step.name)
DescribableModel<? extends Describable> model

if (desc != null) {
model = modelForStep(step.name)
} else {
desc = lookupFunction(step.name)
if (desc != null) {
model = modelForDescribable(step.name)
model = modelForStep(step.name)
} else {
desc = lookupFunction(step.name)
if (desc != null) {
model = modelForDescribable(step.name)
}
}
}

if (model != null) {
if (step.args instanceof ModelASTNamedArgumentList) {
ModelASTNamedArgumentList argList = (ModelASTNamedArgumentList) step.args

argList.arguments.each { k, v ->
def p = model.getParameter(k.key);
if (p==null) {
String possible = EditDistance.findNearest(k.key, model.getParameters().collect { it.name })
errorCollector.error(k, "Invalid parameter '${k.key}', did you mean '${possible}'?")
valid = false
return;
if (model != null) {
if (step.args instanceof ModelASTNamedArgumentList) {
ModelASTNamedArgumentList argList = (ModelASTNamedArgumentList) step.args

argList.arguments.each { k, v ->
def p = model.getParameter(k.key);
if (p == null) {
String possible = EditDistance.findNearest(k.key, model.getParameters().collect {
it.name
})
errorCollector.error(k, "Invalid parameter '${k.key}', did you mean '${possible}'?")
valid = false
return;
}

if (!validateParameterType(v, p.erasedType, k)) {
valid = false
}
}

if (!validateParameterType(v, p.erasedType, k)) {
valid = false
model.parameters.each { p ->
if (p.isRequired() && !argList.containsKeyName(p.name)) {
errorCollector.error(step, "Missing required parameter: '${p.name}'")
valid = false
}
}
}
model.parameters.each { p ->
if (p.isRequired() && !argList.containsKeyName(p.name)) {
errorCollector.error(step, "Missing required parameter: '${p.name}'")
valid = false
}
}
} else {
assert step.args instanceof ModelASTSingleArgument;
ModelASTSingleArgument arg = (ModelASTSingleArgument)step.args;

def p = model.soleRequiredParameter;
if (p==null && !stepTakesClosure(desc)) {
errorCollector.error(step, "Step does not take a single required parameter - use named parameters instead")
valid = false
} else {
Class erasedType = p?.erasedType
if (stepTakesClosure(desc)) {
erasedType = String.class
}
def v = arg.value;
assert step.args instanceof ModelASTSingleArgument;
ModelASTSingleArgument arg = (ModelASTSingleArgument) step.args;

if (!validateParameterType(v, erasedType)) {
def p = model.soleRequiredParameter;
if (p == null && !stepTakesClosure(desc)) {
errorCollector.error(step, "Step does not take a single required parameter - use named parameters instead")
valid = false
} else {
Class erasedType = p?.erasedType
if (stepTakesClosure(desc)) {
erasedType = String.class
}
def v = arg.value;

if (!validateParameterType(v, erasedType)) {
valid = false
}
}
}

}
}
}
}


return valid
}

Expand Down
9 changes: 8 additions & 1 deletion src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/ValidatorTest.java
View file
Expand Up @@ -40,7 +40,14 @@ public class ValidatorTest extends AbstractModelDefTest {
public void rejectStageInSteps() throws Exception {
prepRepoWithJenkinsfile("errors", "rejectStageInSteps");

failWithError("Invalid step 'stage' used - not allowed in this context @ line");
failWithError("Invalid step 'stage' used - not allowed in this context - The stage step cannot be used in step blocks in Pipeline Config");
}

@Test
public void rejectParallelMixedInSteps() throws Exception {
prepRepoWithJenkinsfile("errors", "rejectParallelMixedInSteps");

failWithError("Invalid step 'parallel' used - not allowed in this context - The parallel step can only be used as the only top-level step in a stage's step block");
}

@Ignore("I still want to block parallel, but I'm not sure it's worth it, ignoring for now.")
Expand Down
37 changes: 37 additions & 0 deletions src/test/resources/errors/rejectParallelMixedInSteps.groovy
View file
@@ -0,0 +1,37 @@
/*
* The MIT License
*
* Copyright (c) 2016, CloudBees, Inc.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/

pipeline {
agent none
stages {
stage("foo") {
echo "hello"
parallel([a: "echo '1'",
b: "echo '2'"])
}
}
}



0 comments on commit cb86d2b

Please sign in to comment.