[JENKINS-55612] Add optional label to echo step

[bitwiseman]

JENKINS-55612

This is a minimal implementation for this issue, including some basic tests.

Based on this PR: jenkinsci/workflow-durable-task-step-plugin#93

[dwnusbaum]

In jenkinsci/workflow-durable-task-step-plugin#93 (comment) Jesse raised the point that maybe it would have been better to use StepDescriptor#argumentsToString instead of a label, so maybe we should use that approach instead? I am interested to hear what @abayer thinks.

The main benefit with that approach AIUI is that the label would always be consistently "Print Message" and what was shown next to it would either be the script, if it is short, or the label if it is specified, or nothing if there is no label and the script is too long or multiple arguments are passed.

Also, by adding a second argument to the step but still using the default implementation of argumentsToString, the message won't show up in the Blue Ocean view if a label is specified. Maybe that's fine, or maybe we should update argumentsToString to handle multiple args similarly to ShellStep.

[dwnusbaum]

nit: It feels a little awkward to me to throw an exception here. On the one hand it makes sense to fail fast, but I don't think anyone should be calling this method directly except for Stapler and maybe devs in a unit test, so perhaps adding an @Nonnull annotation would be good enough, and you still have to have the null check in start anyway for builds deserialized from before the label existed so it seems like a bit of a wash. Maybe you could add a doCheckLabel method to the descriptor to do some validation for anyone using the pipeline syntax page, but I'm not sure whether that works well with optional @DataBoundSetters.

[bitwiseman]

@dwnusbaum
Yeah, since I check for null in the start this is overkill. I like the @NonNull annotation for self-documenting code.

[bitwiseman]

@dwnusbaum
As to your general comment, are you pointing to a more general issue that I should file and come back to? Or are you saying I need to change something in this PR?
...

Ah, wait.
I'm looking at the issue description ( https://issues.jenkins-ci.org/browse/JENKINS-55612) again. What the user wants differs from what they end up asking for.

1. They want echo to still display a meaningful label even when the message is long - see this image
2. If there isn't some good way to do this, they'd like to add a label field they can set manually.

Back to the drawing board for a minute.

[bitwiseman]

@dwnusbaum
For that other case I prefer LabelAction. For this one, I'm not sure.

I responded on the other PR, but to summarize: LabelAction is an additional structure and lets the UI decide how to display a step. The string from StepDescriptor#argumentsToString does not give the UI that option.

That aside, as I noted above, the user's core problem is that Blue Ocean only shows the message for short echo messages (it shows the label "Print Message" after the message text). For longer/multiline messages it ignores the message text and just shows "Print Message".

I think adding a label is still the right way to go.

[bitwiseman]

@dwnusbaum I've updated this PR with a version that truncate the argument string to the first line of the message but also adds the label field. I haven't run this up in a local blue ocean yet to verify the behavior.

[jglick]

This PR exemplifies the reason I thought JENKINS-55410 was a bad idea to begin with—it just leads to endless creeping requests to add label to more and more steps, ad-hoc. I still feel that it would be a better design to use stage for the purpose for which it was designed¹: associating a human-readable label with a block of script. If some UI like Blue Ocean does not currently honor nested stages well, fix that UI rather than forcing scripts to work around it. IMO.

And what really is the use case here?

if the message for echo step is too long, it solely prints Print Message in pipeline view

This behavior is not fixed in stone. It is just the current design of ArgumentsAction[Impl] that long arguments are replaced by OVERSIZE_VALUE and the original text completely lost, rather than simply being truncated after newlines, too many characters, etc. so that you would see something like

PhpStorm inspection results: …

without needing to change your script. I actually argued for such a behavior during the design, but it was deferred: jenkinsci/workflow-api-plugin#26 (comment)

---

¹ Actually my original choice of step name was label, leaving the original behavior of stage untouched and deprecating it, but @kohsuke overrode me on that.