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

Erroneous handling of BodyInvoker.withDisplayName

    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 ]
            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 ]
            scm_issue_link SCM/JIRA link daemon made changes -
            Status In Progress [ 3 ] Resolved [ 5 ]
            Resolution Fixed [ 1 ]
            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: