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

Erroneous handling of BodyInvoker.withDisplayName

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      In https://github.com/jenkinsci/workflow-plugin/commit/e24a64a7a6404be97f42340f4c44d3988be1052c you added

      createBodyBlockNode = (name==null);
      

      Is this not backwards? The Javadoc says that if name == null, the body is "anonymous", and

      anonymous body invocation suppress the generation of BlockStartNode/BlockEndNode

      which makes sense. Yet here you are saying that an anonymous body should create a body block node, which looks like a mistake. There appears to be no call to this method passing an actual display name anywhere in the codebase, and no test of how it should behave. I suspect that a call to non-null displayName would actually produce an IllegalStateException in start, since then startNodeActions would have a LabelAction. Note that ParallelStepExecution does call withStartAction, the only one to do so, and effectively sets a display name for the block, yet not using regular LabelAction; it seems the only purpose for the custom subclass is to assert branch names in a test.

      Furthermore, displayName is null by default, yet the Javadoc explicitly suggests calling withDisplayName(null), which will have no effect since createBodyBlockNode is also true by default. And many—but not all—steps indeed call withDisplayName(null).

      This all seems confused. Either blocks should be anonymous by default, in which case createBodyBlockNode should default to false, and all the calls to withDisplayName(null) should be deleted, and the name parameter should be marked @Nonnull, and the Javadoc should confirm the default behavior, and ParallelStepExecution should be handled somehow; or blocks should be visible by default, in which case displayName should have some default value (perhaps computed according to StepDescriptor.displayName?) and you would need to call withDisplayName(null) to turn it off. The first option seems more appropriate to me.

        Attachments

          Issue Links

            Activity

            jglick Jesse Glick created issue -
            jglick Jesse Glick made changes -
            Field Original Value New Value
            Link This issue is blocking JENKINS-26107 [ JENKINS-26107 ]
            Hide
            jglick Jesse Glick added a comment -

            May make it feasible to make waitUntil be less verbose.

            Show
            jglick Jesse Glick added a comment - May make it feasible to make waitUntil be less verbose.
            jglick Jesse Glick made changes -
            Link This issue is blocking JENKINS-30088 [ JENKINS-30088 ]
            jglick Jesse Glick made changes -
            Assignee Kohsuke Kawaguchi [ kohsuke ]
            jglick Jesse Glick made changes -
            Link This issue is related to JENKINS-30088 [ JENKINS-30088 ]
            jglick Jesse Glick made changes -
            Link This issue is related to JENKINS-30088 [ JENKINS-30088 ]
            jglick Jesse Glick made changes -
            Status Open [ 1 ] In Progress [ 3 ]
            jglick Jesse Glick made changes -
            Assignee Jesse Glick [ jglick ]
            jglick Jesse Glick made changes -
            Remote Link This issue links to "workflow-cps PR 3 (Web Link)" [ 14179 ]
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Jesse Glick
            Path:
            src/main/java/org/jenkinsci/plugins/workflow/cps/CpsBodyExecution.java
            src/main/java/org/jenkinsci/plugins/workflow/cps/CpsBodyInvoker.java
            src/test/java/org/jenkinsci/plugins/workflow/DynamicEnvironmentExpanderTest.java
            http://jenkins-ci.org/commit/workflow-cps-plugin/ff4ab3e544a346400aaead547565f29d99c74990
            Log:
            [FIXED JENKINS-26156] Fixed BodyInvoker.withDisplayName implementation to allow a non-null argument.

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: src/main/java/org/jenkinsci/plugins/workflow/cps/CpsBodyExecution.java src/main/java/org/jenkinsci/plugins/workflow/cps/CpsBodyInvoker.java src/test/java/org/jenkinsci/plugins/workflow/DynamicEnvironmentExpanderTest.java http://jenkins-ci.org/commit/workflow-cps-plugin/ff4ab3e544a346400aaead547565f29d99c74990 Log: [FIXED JENKINS-26156] Fixed BodyInvoker.withDisplayName implementation to allow a non-null argument.
            scm_issue_link SCM/JIRA link daemon made changes -
            Status In Progress [ 3 ] Resolved [ 5 ]
            Resolution Fixed [ 1 ]
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Jesse Glick
            Path:
            src/main/java/org/jenkinsci/plugins/workflow/cps/CpsBodyExecution.java
            src/main/java/org/jenkinsci/plugins/workflow/cps/CpsBodyInvoker.java
            src/main/java/org/jenkinsci/plugins/workflow/cps/nodes/StepEndNode.java
            src/main/java/org/jenkinsci/plugins/workflow/cps/nodes/StepStartNode.java
            src/test/java/org/jenkinsci/plugins/workflow/DynamicEnvironmentExpanderTest.java
            src/test/java/org/jenkinsci/plugins/workflow/WorkflowJobNonRestartingTest.java
            http://jenkins-ci.org/commit/workflow-cps-plugin/5e8cbc2be2b340c2532d4720c7a24ca44d53c4c3
            Log:
            Merge pull request #3 from jglick/JENKINS-26156-BodyInvoker.withDisplayName

            JENKINS-26156 BodyInvoker.displayName was broken

            Compare: https://github.com/jenkinsci/workflow-cps-plugin/compare/654e8b4fdbde...5e8cbc2be2b3

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: src/main/java/org/jenkinsci/plugins/workflow/cps/CpsBodyExecution.java src/main/java/org/jenkinsci/plugins/workflow/cps/CpsBodyInvoker.java src/main/java/org/jenkinsci/plugins/workflow/cps/nodes/StepEndNode.java src/main/java/org/jenkinsci/plugins/workflow/cps/nodes/StepStartNode.java src/test/java/org/jenkinsci/plugins/workflow/DynamicEnvironmentExpanderTest.java src/test/java/org/jenkinsci/plugins/workflow/WorkflowJobNonRestartingTest.java http://jenkins-ci.org/commit/workflow-cps-plugin/5e8cbc2be2b340c2532d4720c7a24ca44d53c4c3 Log: Merge pull request #3 from jglick/ JENKINS-26156 -BodyInvoker.withDisplayName JENKINS-26156 BodyInvoker.displayName was broken Compare: https://github.com/jenkinsci/workflow-cps-plugin/compare/654e8b4fdbde...5e8cbc2be2b3
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Jesse Glick
            Path:
            src/main/java/org/jenkinsci/plugins/workflow/steps/CatchErrorStep.java
            src/main/java/org/jenkinsci/plugins/workflow/steps/PushdStep.java
            src/main/java/org/jenkinsci/plugins/workflow/steps/TimeoutStepExecution.java
            http://jenkins-ci.org/commit/workflow-basic-steps-plugin/a14ab59ff8445f6341b2fbb7e0e8ccbef04eedec
            Log:
            JENKINS-26156 Calling BodyInvoker.withDisplayName(null) was always a no-op, and will soon be a FindBugs warning.
            (cherry picked from commit 7806a767f7f4253200b588f81858c07139ce6fdb)

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: src/main/java/org/jenkinsci/plugins/workflow/steps/CatchErrorStep.java src/main/java/org/jenkinsci/plugins/workflow/steps/PushdStep.java src/main/java/org/jenkinsci/plugins/workflow/steps/TimeoutStepExecution.java http://jenkins-ci.org/commit/workflow-basic-steps-plugin/a14ab59ff8445f6341b2fbb7e0e8ccbef04eedec Log: JENKINS-26156 Calling BodyInvoker.withDisplayName(null) was always a no-op, and will soon be a FindBugs warning. (cherry picked from commit 7806a767f7f4253200b588f81858c07139ce6fdb)
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Jesse Glick
            Path:
            src/main/java/org/jenkinsci/plugins/workflow/steps/CatchErrorStep.java
            src/main/java/org/jenkinsci/plugins/workflow/steps/PushdStep.java
            src/main/java/org/jenkinsci/plugins/workflow/steps/TimeoutStepExecution.java
            http://jenkins-ci.org/commit/workflow-basic-steps-plugin/7360c8d48bdb0f4ac2fb4aa4f8ac1e7040faf7cb
            Log:
            Merge pull request #6 from jglick/cleanup-JENKINS-26156

            JENKINS-26156 Remove calls to BodyInvoker.withDisplayName(null)

            Compare: https://github.com/jenkinsci/workflow-basic-steps-plugin/compare/3d258572897d...7360c8d48bdb

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: src/main/java/org/jenkinsci/plugins/workflow/steps/CatchErrorStep.java src/main/java/org/jenkinsci/plugins/workflow/steps/PushdStep.java src/main/java/org/jenkinsci/plugins/workflow/steps/TimeoutStepExecution.java http://jenkins-ci.org/commit/workflow-basic-steps-plugin/7360c8d48bdb0f4ac2fb4aa4f8ac1e7040faf7cb Log: Merge pull request #6 from jglick/cleanup- JENKINS-26156 JENKINS-26156 Remove calls to BodyInvoker.withDisplayName(null) Compare: https://github.com/jenkinsci/workflow-basic-steps-plugin/compare/3d258572897d...7360c8d48bdb
            rtyler R. Tyler Croy made changes -
            Workflow JNJira [ 160147 ] JNJira + In-Review [ 196338 ]
            abayer Andrew Bayer made changes -
            Component/s pipeline-general [ 21692 ]
            abayer Andrew Bayer made changes -
            Component/s workflow-plugin [ 18820 ]

              People

              • Assignee:
                jglick Jesse Glick
                Reporter:
                jglick Jesse Glick
              • Votes:
                2 Vote for this issue
                Watchers:
                8 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: