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

Better error messages when illegal identifiers are used in an `environment` block

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      Presumably environment variable names must be legal Groovy identifiers (although generally, I don't think environment variables have as a strict a naming convention).

      When trying to use various illegal identifiers, I got various different error messages:

      pipeline {
        agent any
        stages {
          stage('Foo') {
              environment {
                // Failure: Expected name=value pairs
                3D_GLASSES = 'no'
                
                // Failure: Expected name=value pairs
                123ABC = 'bad'
      
                // Failure: expecting token in range: '0'..'9', found 'X'
                456_XYZ = 'numeric literal?'
      
                // Failure: ((what - about) - hyphens) is a binary expression
                what-about-hyphens = 'binary expression?'
      
                // OK
                LEGAL_GROOVY_IDENTIFIER = 'yay'
              }
              steps {
                sh 'env'
              }
          }
        }
      }
      

      I would expect some linting when editing the Pipeline (well, the editor does catch the 456_XYZ case), and nicer error messages at runtime.

        Attachments

          Issue Links

            Activity

            Hide
            orrc Christopher Orr added a comment -

            Another one is quoted names:

            environment {
              // Failure [what-about-single-quotes] is a constant expression, but it should be a variable 
              'what-about-single-quotes' = 'unlikely'
            }
            
            Show
            orrc Christopher Orr added a comment - Another one is quoted names: environment { // Failure [what-about-single-quotes] is a constant expression, but it should be a variable 'what-about-single-quotes' = 'unlikely' }
            Hide
            abayer Andrew Bayer added a comment -

            Argh, I only actually did the check for being a valid Java identifier - https://github.com/jenkinsci/pipeline-model-definition-plugin/commit/e3d0a948f28975a595f41d0749907c788bfb0306 - that was relevant when coming from the JSON. D'oh.

            Show
            abayer Andrew Bayer added a comment - Argh, I only actually did the check for being a valid Java identifier - https://github.com/jenkinsci/pipeline-model-definition-plugin/commit/e3d0a948f28975a595f41d0749907c788bfb0306 - that was relevant when coming from the JSON. D'oh.
            Hide
            abayer Andrew Bayer added a comment -

            Ok, 456_XYZ I can't do anything about - that's a Groovy compilation error, so it's happening before we get to the linting.

            Show
            abayer Andrew Bayer added a comment - Ok, 456_XYZ I can't do anything about - that's a Groovy compilation error, so it's happening before we get to the linting.
            Hide
            abayer Andrew Bayer added a comment -

            Same with what-about-hyphens. Boo.

            Show
            abayer Andrew Bayer added a comment - Same with what-about-hyphens . Boo.
            Hide
            abayer Andrew Bayer added a comment -

            Ok, I did do the check right, I'm just not reporting it right. Oy.

            Show
            abayer Andrew Bayer added a comment - Ok, I did do the check right, I'm just not reporting it right. Oy.
            Show
            abayer Andrew Bayer added a comment - PR up at https://github.com/jenkinsci/pipeline-model-definition-plugin/pull/106
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Andrew Bayer
            Path:
            pipeline-model-definition/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/parser/ModelParser.groovy
            pipeline-model-definition/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/Messages.properties
            pipeline-model-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/ValidatorTest.java
            pipeline-model-definition/src/test/resources/errors/invalidEnvironmentIdentifiers.groovy
            http://jenkins-ci.org/commit/pipeline-model-definition-plugin/fffa3bca1d7b0559bb166d78f95d1dd3b8da9dff
            Log:
            JENKINS-41645 Better validation for non-binary expressions in env

            We can't handle all invalid environment identifiers currently - things
            like 123_ABC or something-with-hyphens or 'somethingInQuotes' all end
            up failing out at initial Groovy compilation, before we get the AST to
            work with. We do catch those when coming from JSON, which is
            something. But until this, we didn't give useful errors for any
            invalid environment identifier that actually gets through compilation

            • i.e., 3D_GLASSES, 123ABC. Those aren't BinaryExpressions, so they
              were skipped and we ended up with just the general error case
              reporting. This commit changes that. =)
            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Andrew Bayer Path: pipeline-model-definition/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/parser/ModelParser.groovy pipeline-model-definition/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/Messages.properties pipeline-model-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/ValidatorTest.java pipeline-model-definition/src/test/resources/errors/invalidEnvironmentIdentifiers.groovy http://jenkins-ci.org/commit/pipeline-model-definition-plugin/fffa3bca1d7b0559bb166d78f95d1dd3b8da9dff Log: JENKINS-41645 Better validation for non-binary expressions in env We can't handle all invalid environment identifiers currently - things like 123_ABC or something-with-hyphens or 'somethingInQuotes' all end up failing out at initial Groovy compilation, before we get the AST to work with. We do catch those when coming from JSON, which is something. But until this, we didn't give useful errors for any invalid environment identifier that actually gets through compilation i.e., 3D_GLASSES, 123ABC. Those aren't BinaryExpressions, so they were skipped and we ended up with just the general error case reporting. This commit changes that. =)
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Andrew Bayer
            Path:
            pipeline-model-definition/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/parser/ModelParser.groovy
            pipeline-model-definition/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/Messages.properties
            pipeline-model-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/EnvironmentTest.java
            pipeline-model-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/ValidatorTest.java
            pipeline-model-definition/src/test/resources/errors/envIdentifierDigitsUnderscore.groovy
            pipeline-model-definition/src/test/resources/errors/envIdentifierHyphens.groovy
            pipeline-model-definition/src/test/resources/errors/envIdentifierStartingWithDigit.groovy
            pipeline-model-definition/src/test/resources/errors/envIdentifierString.groovy
            pipeline-model-definition/src/test/resources/errors/envIdentifiersCaughtInternally.groovy
            pipeline-model-definition/src/test/resources/simpleEnvironment.groovy
            http://jenkins-ci.org/commit/pipeline-model-definition-plugin/4b8ddf1e03419ec762cfd74ffa0fe0860f32d0e6
            Log:
            Merge pull request #106 from abayer/jenkins-41645

            JENKINS-41645 Better validation for non-binary expressions in env

            Compare: https://github.com/jenkinsci/pipeline-model-definition-plugin/compare/20f8126272bb...4b8ddf1e0341

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Andrew Bayer Path: pipeline-model-definition/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/parser/ModelParser.groovy pipeline-model-definition/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/Messages.properties pipeline-model-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/EnvironmentTest.java pipeline-model-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/ValidatorTest.java pipeline-model-definition/src/test/resources/errors/envIdentifierDigitsUnderscore.groovy pipeline-model-definition/src/test/resources/errors/envIdentifierHyphens.groovy pipeline-model-definition/src/test/resources/errors/envIdentifierStartingWithDigit.groovy pipeline-model-definition/src/test/resources/errors/envIdentifierString.groovy pipeline-model-definition/src/test/resources/errors/envIdentifiersCaughtInternally.groovy pipeline-model-definition/src/test/resources/simpleEnvironment.groovy http://jenkins-ci.org/commit/pipeline-model-definition-plugin/4b8ddf1e03419ec762cfd74ffa0fe0860f32d0e6 Log: Merge pull request #106 from abayer/jenkins-41645 JENKINS-41645 Better validation for non-binary expressions in env Compare: https://github.com/jenkinsci/pipeline-model-definition-plugin/compare/20f8126272bb...4b8ddf1e0341

              People

              • Assignee:
                abayer Andrew Bayer
                Reporter:
                orrc Christopher Orr
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: