Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-37788

Pipeline Config: Eliminate the "isConstant" AST/JSON field

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      We've got a field for values called "isConstant" that doesn't serve a very clear purpose - right now, it's basically just used in two places - to determine whether to actually try type-casting in validation and to decide which subtype of ConfigASTValue is used when parsing from JSON. So...we probably don't need it, or if we do need it, we need to actually do something more useful and determinant with it.

      In the first case, which is probably the one that actually matters, since that's what cares which ConfigASTValue subtype is used, we use it to determine whether to try casting for parameter validation. Without that check, we end up with errors for things like [$class: ...] legacy metastep syntax, which are String when they get to us in validation currently, so can't be cast to Map as they'd need to be. So beyond the confusion of isConstant, we're also not actually doing validation of legacy metastep syntax!

        Attachments

          Issue Links

            Activity

            Hide
            abayer Andrew Bayer added a comment -

            This needs more discussion with Kohsuke Kawaguchi to understand what it's intended for in the first place. I'm resurrecting my month-old experiments on removing it, but I think they broke. =)

            Show
            abayer Andrew Bayer added a comment - This needs more discussion with Kohsuke Kawaguchi to understand what it's intended for in the first place. I'm resurrecting my month-old experiments on removing it, but I think they broke. =)
            Hide
            abayer Andrew Bayer added a comment -

            Aaaand yup, specifically echo('['+acmeVar.baz()+']'), which is valid by the declarative subset, doesn't work right. We do need isConstant so we can know whether the value is a literal or not. If it's not a literal, we need to know that so we can evaluate it.

            Show
            abayer Andrew Bayer added a comment - Aaaand yup, specifically echo(' ['+acmeVar.baz()+'] ') , which is valid by the declarative subset, doesn't work right. We do need isConstant so we can know whether the value is a literal or not. If it's not a literal, we need to know that so we can evaluate it.
            Hide
            michaelneale Michael Neale added a comment -

            This won't work with a graphical editor (think about it for a bit)

            Show
            michaelneale Michael Neale added a comment - This won't work with a graphical editor (think about it for a bit)
            Hide
            michaelneale Michael Neale added a comment -

            Actually this may be able to work... and be useful even (Andrew has explained what it means to me). I take it back.

            Show
            michaelneale Michael Neale added a comment - Actually this may be able to work... and be useful even (Andrew has explained what it means to me). I take it back.
            Hide
            abayer Andrew Bayer added a comment -

            I'm going to rename this to "isLiteral" since that's a more accurate/meaningful description and then we'll close this.

            Show
            abayer Andrew Bayer added a comment - I'm going to rename this to "isLiteral" since that's a more accurate/meaningful description and then we'll close this.
            Hide
            abayer Andrew Bayer added a comment -

            PR changing from isConstant to isLiteral up at https://github.com/jenkinsci/pipeline-model-definition-plugin/pull/25

            Show
            abayer Andrew Bayer added a comment - PR changing from isConstant to isLiteral up at https://github.com/jenkinsci/pipeline-model-definition-plugin/pull/25
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Andrew Bayer
            Path:
            src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTValue.groovy
            src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/parser/JSONParser.groovy
            src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidator.groovy
            src/main/resources/ast-schema.json
            src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/AbstractModelDefTest.java
            src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/validator/JSONValidationTest.java
            src/test/resources/json/agentAny.json
            src/test/resources/json/agentDocker.json
            src/test/resources/json/agentLabel.json
            src/test/resources/json/agentNoneWithNode.json
            src/test/resources/json/errors/emptyEnvironment.json
            src/test/resources/json/errors/emptyNotifications.json
            src/test/resources/json/errors/emptyParallel.json
            src/test/resources/json/errors/emptyPostBuild.json
            src/test/resources/json/errors/emptyStages.json
            src/test/resources/json/errors/invalidBuildCondition.json
            src/test/resources/json/errors/malformed.json
            src/test/resources/json/errors/missingAgent.json
            src/test/resources/json/errors/missingRequiredStepParameters.json
            src/test/resources/json/errors/missingStages.json
            src/test/resources/json/errors/notInstalledToolType.json
            src/test/resources/json/errors/notInstalledToolVersion.json
            src/test/resources/json/errors/rejectParallelMixedInSteps.json
            src/test/resources/json/errors/rejectStageInSteps.json
            src/test/resources/json/errors/stageWithoutName.json
            src/test/resources/json/errors/unknownStepParameter.json
            src/test/resources/json/errors/unlistedToolType.json
            src/test/resources/json/globalLibrarySuccess.json
            src/test/resources/json/legacyMetaStepSyntax.json
            src/test/resources/json/metaStepSyntax.json
            src/test/resources/json/parallelPipeline.json
            src/test/resources/json/postBuildAndNotifications.json
            src/test/resources/json/simpleEnvironment.json
            src/test/resources/json/simpleNotification.json
            src/test/resources/json/simplePipeline.json
            src/test/resources/json/simplePostBuild.json
            src/test/resources/json/simpleScript.json
            src/test/resources/json/simpleTools.json
            src/test/resources/json/twoStagePipeline.json
            src/test/resources/json/validStepParameters.json
            http://jenkins-ci.org/commit/pipeline-model-definition-plugin/670ecdb55ea6b5ab7e9083fd44079af61fe229c0
            Log:
            [FIXED JENKINS-37788] Use isLiteral instead of isConstant.

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Andrew Bayer Path: src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTValue.groovy src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/parser/JSONParser.groovy src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidator.groovy src/main/resources/ast-schema.json src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/AbstractModelDefTest.java src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/validator/JSONValidationTest.java src/test/resources/json/agentAny.json src/test/resources/json/agentDocker.json src/test/resources/json/agentLabel.json src/test/resources/json/agentNoneWithNode.json src/test/resources/json/errors/emptyEnvironment.json src/test/resources/json/errors/emptyNotifications.json src/test/resources/json/errors/emptyParallel.json src/test/resources/json/errors/emptyPostBuild.json src/test/resources/json/errors/emptyStages.json src/test/resources/json/errors/invalidBuildCondition.json src/test/resources/json/errors/malformed.json src/test/resources/json/errors/missingAgent.json src/test/resources/json/errors/missingRequiredStepParameters.json src/test/resources/json/errors/missingStages.json src/test/resources/json/errors/notInstalledToolType.json src/test/resources/json/errors/notInstalledToolVersion.json src/test/resources/json/errors/rejectParallelMixedInSteps.json src/test/resources/json/errors/rejectStageInSteps.json src/test/resources/json/errors/stageWithoutName.json src/test/resources/json/errors/unknownStepParameter.json src/test/resources/json/errors/unlistedToolType.json src/test/resources/json/globalLibrarySuccess.json src/test/resources/json/legacyMetaStepSyntax.json src/test/resources/json/metaStepSyntax.json src/test/resources/json/parallelPipeline.json src/test/resources/json/postBuildAndNotifications.json src/test/resources/json/simpleEnvironment.json src/test/resources/json/simpleNotification.json src/test/resources/json/simplePipeline.json src/test/resources/json/simplePostBuild.json src/test/resources/json/simpleScript.json src/test/resources/json/simpleTools.json src/test/resources/json/twoStagePipeline.json src/test/resources/json/validStepParameters.json http://jenkins-ci.org/commit/pipeline-model-definition-plugin/670ecdb55ea6b5ab7e9083fd44079af61fe229c0 Log: [FIXED JENKINS-37788] Use isLiteral instead of isConstant.
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Andrew Bayer
            Path:
            src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTValue.groovy
            src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/parser/JSONParser.groovy
            src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidator.groovy
            src/main/resources/ast-schema.json
            src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/AbstractModelDefTest.java
            src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/EnvironmentTest.java
            src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/validator/JSONValidationTest.java
            src/test/resources/json/agentAny.json
            src/test/resources/json/agentDocker.json
            src/test/resources/json/agentLabel.json
            src/test/resources/json/agentNoneWithNode.json
            src/test/resources/json/errors/emptyEnvironment.json
            src/test/resources/json/errors/emptyNotifications.json
            src/test/resources/json/errors/emptyParallel.json
            src/test/resources/json/errors/emptyPostBuild.json
            src/test/resources/json/errors/emptyStages.json
            src/test/resources/json/errors/invalidBuildCondition.json
            src/test/resources/json/errors/malformed.json
            src/test/resources/json/errors/missingAgent.json
            src/test/resources/json/errors/missingRequiredStepParameters.json
            src/test/resources/json/errors/missingStages.json
            src/test/resources/json/errors/notInstalledToolType.json
            src/test/resources/json/errors/notInstalledToolVersion.json
            src/test/resources/json/errors/rejectParallelMixedInSteps.json
            src/test/resources/json/errors/rejectStageInSteps.json
            src/test/resources/json/errors/stageWithoutName.json
            src/test/resources/json/errors/unknownStepParameter.json
            src/test/resources/json/errors/unlistedToolType.json
            src/test/resources/json/globalLibrarySuccess.json
            src/test/resources/json/legacyMetaStepSyntax.json
            src/test/resources/json/metaStepSyntax.json
            src/test/resources/json/parallelPipeline.json
            src/test/resources/json/postBuildAndNotifications.json
            src/test/resources/json/simpleEnvironment.json
            src/test/resources/json/simpleNotification.json
            src/test/resources/json/simplePipeline.json
            src/test/resources/json/simplePostBuild.json
            src/test/resources/json/simpleScript.json
            src/test/resources/json/simpleTools.json
            src/test/resources/json/twoStagePipeline.json
            src/test/resources/json/validStepParameters.json
            http://jenkins-ci.org/commit/pipeline-model-definition-plugin/366510af23854215f7ed19869ff1ee0f1e6e45de
            Log:
            Merge pull request #25 from abayer/jenkins-37788-mk2

            [FIXED JENKINS-37788] Use isLiteral instead of isConstant.

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Andrew Bayer Path: src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTValue.groovy src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/parser/JSONParser.groovy src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidator.groovy src/main/resources/ast-schema.json src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/AbstractModelDefTest.java src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/EnvironmentTest.java src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/validator/JSONValidationTest.java src/test/resources/json/agentAny.json src/test/resources/json/agentDocker.json src/test/resources/json/agentLabel.json src/test/resources/json/agentNoneWithNode.json src/test/resources/json/errors/emptyEnvironment.json src/test/resources/json/errors/emptyNotifications.json src/test/resources/json/errors/emptyParallel.json src/test/resources/json/errors/emptyPostBuild.json src/test/resources/json/errors/emptyStages.json src/test/resources/json/errors/invalidBuildCondition.json src/test/resources/json/errors/malformed.json src/test/resources/json/errors/missingAgent.json src/test/resources/json/errors/missingRequiredStepParameters.json src/test/resources/json/errors/missingStages.json src/test/resources/json/errors/notInstalledToolType.json src/test/resources/json/errors/notInstalledToolVersion.json src/test/resources/json/errors/rejectParallelMixedInSteps.json src/test/resources/json/errors/rejectStageInSteps.json src/test/resources/json/errors/stageWithoutName.json src/test/resources/json/errors/unknownStepParameter.json src/test/resources/json/errors/unlistedToolType.json src/test/resources/json/globalLibrarySuccess.json src/test/resources/json/legacyMetaStepSyntax.json src/test/resources/json/metaStepSyntax.json src/test/resources/json/parallelPipeline.json src/test/resources/json/postBuildAndNotifications.json src/test/resources/json/simpleEnvironment.json src/test/resources/json/simpleNotification.json src/test/resources/json/simplePipeline.json src/test/resources/json/simplePostBuild.json src/test/resources/json/simpleScript.json src/test/resources/json/simpleTools.json src/test/resources/json/twoStagePipeline.json src/test/resources/json/validStepParameters.json http://jenkins-ci.org/commit/pipeline-model-definition-plugin/366510af23854215f7ed19869ff1ee0f1e6e45de Log: Merge pull request #25 from abayer/jenkins-37788-mk2 [FIXED JENKINS-37788] Use isLiteral instead of isConstant.

              People

              • Assignee:
                abayer Andrew Bayer
                Reporter:
                abayer Andrew Bayer
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: