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

Improve validation around script blocks in editor or remove script blocks

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: Resolved (View Workflow)
    • Priority: Major
    • Resolution: Fixed
    • Environment:
    • Sprint:
      Blue Ocean 1.4 - beta 1, Blue Ocean 1.4 - beta 2
    • Similar Issues:

      Description

      In some cases you can make an invalid edit to a script block and it will show a warning, but it may be illegible (it is possbile that some cases slip through the validator and the user ends up with a non valid declarative file and the visual editor can be invoked). 

      Karl Shultz would be good if we could try this again (I build from master) to see if we can see any case where going from the editor gets to a non re-openable state (editing it by hand - well all bets are off in that case anyway). 

       

            • original from karl follows ---*

      Summary:
      Given an existing, working Jenkinsfile which includes two parallel stages (supported by Declarative 1.2-beta4), saving changes to this Jenkinsfile from the Blue Ocean editor (1.3-beta2) is inop.

      Frequency:
      Not 100% reproducible, even with the same repos and branches.

      Prerequisites:
      1. A known-good Jenkinsfile in a remote repository, which includes at least one pair of parallel stages that are compliant with Declarative 1.2-beta4. I'm testing with a GitHub enterprise server.

      2. This Jenkinsfile should be on a branch other than master. Shown below, the branch I'm working on, verify-parallel-lengths, is favorited:

      3. A sample Jenkinsfile with which to recreate this problem is shown below:

      pipeline {
          agent any
          stages { 
              stage('Each of these takes ten seconds') {
                  parallel {
                      stage('first') {
                          steps {
                              echo "First branch"
                                  script {
                                      sh 'for i in `seq 1 10`; do echo "Sleeping 1S"; sleep 1; done'
                                  }
                          }
                      }
                      stage('second') {
                          steps {
                              echo "Second branch"
                              script {
                                  sh 'for i in `seq 1 10`; do echo "Sleeping 1S"; sleep 1; done'
                              }
                          }
                      }
                  }
              }
          }
      }
      

      Steps to recreate:
      1. Add the Pipeline via Blue Ocean, and verify that it will run to completion:

      2. Edit the Pipeline using whatever your preferred way to launch the editor is. Here, I'm clicking on the Edit icon in the build that just ran:

      3. Make a trivial change. Here, I'm changing the name of the "wrapper" stage for my two parallel stages:

      4. Click Save. Provide a description of the change, then click "Save and Run." In this example I'm attempting to commit the changes straight to verify-parallel-lengths, but choosing a new branch name also demonstrates the problem:

      5. You'll get an "Error - Bad request" message, and the change won't get saved.

      6. I was not able to recreate this when editing master.

      I will run back through this with the dev console open when I'm not multitasking taking screen shots, and grab a HAR file.

        Attachments

          Issue Links

            Activity

            Hide
            abayer Andrew Bayer added a comment -

            Looks to be a quote round-tripping issue, first and foremost. Digging into it.

            Show
            abayer Andrew Bayer added a comment - Looks to be a quote round-tripping issue, first and foremost. Digging into it.
            Hide
            abayer Andrew Bayer added a comment -

            Oh, wait, you already fixed that part. ha.

            Show
            abayer Andrew Bayer added a comment - Oh, wait, you already fixed that part. ha.
            Hide
            kzantow Keith Zantow added a comment - - edited

            Yeah, see this comment - these errors are displayed as global errors. It would be nice to associate them with the script block, but I'm assuming what's happening is described in that comment.

            Show
            kzantow Keith Zantow added a comment - - edited Yeah, see this comment - these errors are displayed as global errors. It would be nice to associate them with the script block, but I'm assuming what's happening is described in that comment.
            Hide
            abayer Andrew Bayer added a comment -

            PR up at https://github.com/jenkinsci/pipeline-model-definition-plugin/pull/216 - it'll do a quick Groovy parsing phase on the contents of script or when/expression and, if any errors are found in parsing (i.e., syntax errors like mismatched quotes), it'll error out on that location, reporting back that there was a Groovy compilation error. Can't get more detail on the error into that message unless we want a big chunk of text (i.e., the full compilation error, with Groovy source location and all that jazz), but this at least gives a sense of what went wrong.

            It also won't catch errors due to, say, referencing unavailable classes and the like - which is ok, since I don't think any of those scenarios have a real use case in the editor anyway.

            Show
            abayer Andrew Bayer added a comment - PR up at https://github.com/jenkinsci/pipeline-model-definition-plugin/pull/216 - it'll do a quick Groovy parsing phase on the contents of script or when/expression and, if any errors are found in parsing (i.e., syntax errors like mismatched quotes), it'll error out on that location, reporting back that there was a Groovy compilation error. Can't get more detail on the error into that message unless we want a big chunk of text (i.e., the full compilation error, with Groovy source location and all that jazz), but this at least gives a sense of what went wrong. It also won't catch errors due to, say, referencing unavailable classes and the like - which is ok, since I don't think any of those scenarios have a real use case in the editor anyway.
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Andrew Bayer
            Path:
            pipeline-model-api/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTScriptBlock.java
            pipeline-model-api/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTWhenExpression.java
            pipeline-model-definition/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidatorImpl.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/validator/JSONValidationTest.java
            pipeline-model-definition/src/test/resources/json/errors/quoteRoundTripping.json
            http://jenkins-ci.org/commit/pipeline-model-definition-plugin/54b18a34fad2979be7ab76ec6caa9963fa866433
            Log:
            [FIXED JENKINS-46854] Do parsing pass on script/expression blocks in validation

            This doesn't catch all possible script/expression errors that'll show
            up in full Groovy parsing and validation, but it'll catch errors like
            mismatched quotes in JSON form and report back their location and that
            compilation failed. Getting a more granular error would be too much
            text, so I opted for just the "hey, this wasn't valid Groovy syntax"
            approach.

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Andrew Bayer Path: pipeline-model-api/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTScriptBlock.java pipeline-model-api/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTWhenExpression.java pipeline-model-definition/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidatorImpl.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/validator/JSONValidationTest.java pipeline-model-definition/src/test/resources/json/errors/quoteRoundTripping.json http://jenkins-ci.org/commit/pipeline-model-definition-plugin/54b18a34fad2979be7ab76ec6caa9963fa866433 Log: [FIXED JENKINS-46854] Do parsing pass on script/expression blocks in validation This doesn't catch all possible script/expression errors that'll show up in full Groovy parsing and validation, but it'll catch errors like mismatched quotes in JSON form and report back their location and that compilation failed. Getting a more granular error would be too much text, so I opted for just the "hey, this wasn't valid Groovy syntax" approach.

              People

              • Assignee:
                abayer Andrew Bayer
                Reporter:
                kshultz Karl Shultz
              • Votes:
                0 Vote for this issue
                Watchers:
                7 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: