Thanks for the PR!

After #92 sh/bat steps in Blue Ocean should at least show the contents of the script in all cases, see #92 (comment) for a screenshot, but it still seems like this would still be a useful feature.

We try to track new features through Jira tickets, so would you be able to open a ticket here with component workflow-durable-task-step, basically copying the description of your PR to describe what the feature is and why you want it?

Hey,

thanks for the fast feedback. Due i'm not so familiar with the development process i've just created the PR. I've now created the ticket https://issues.jenkins-ci.org/browse/JENKINS-55410. Are there some more steps for me to do?

Could you attach screenshots of what this makes Blue Ocean look like? Thanks.

Hey,
this pipeline
node{ bat label: 'Generate and compile manual', script: 'buildManual.cmd' }

procueds this blue-ocean view

and this step view

After adding the size limit for the label the jenkins build fails with this error:

C:\Jenkins\workspace\w-durable-task-step-plugin_PR-93\target\tmp\jkh2444334337676798093\jobs\foo\builds\1\log: The process cannot access the file because it is being used by another process.

Which seems to me like any kind of flakeyness. The newly added testcase labelShortened has passed.
Any ideas whats going wrong? I've no one...

Which seems to me like any kind of flakeyness. The newly added testcase labelShortened has passed.
Any ideas whats going wrong? I've no one...

Yeah it seems like something flaky on the CI instance to me. I'll close this PR and reopen it to restart the build.

LGTM (the UX PM)

Thanks for the improvement @soenkekueper!

Would it not be better for the label to be used as the return value of argumentsToString and skip the LabelAction?

@jglick @dwnusbaum
I'm not sure.

It looks like the derived classes each already override argumentsToString. Overriding it in this base class would require changes to the derived classes as well. Which could make sense given all the derived classes have a script field. We could pull the field into the base class along with the argumentsToString method. But that would have been a broader change and one that @soenkekueper might not have been aware of.

Looking at the output images in in the comment above, it seems like label and argumentsToString are both displayed in Blue Ocean (though I'd expect the label to be first - shrug). In the classic UI table view, the label is shown in one column and the arguments are shown in another.

The LabelAction adds flexibility and gives more information to the presentation layer to make choices is it sees fit.

@bitwiseman

Overriding it in this base class would require changes to the derived classes as well.

Sure, this would be a two-minute refactoring. The question is about the user expectation. I would have expected a label to override the actual arguments, so that if I write, say,

sh label: 'Collect frobnitzes', script: '''
mvn -B -Dsome.long.option -Dsome.even.longer.option -f whatever/subdir org.whatever:frobnitz-maven-plugin:1.0.3-beta-99:collect
'''

that all compliant UIs will display just Collect frobnitzes and not the crazy command line. (Currently, if you omit set +x / @echo off, the command line will actually get echoed by the shell if you care to look at the step’s log text, so there is no loss of diagnostic ability if something goes wrong.)

As noted in jenkinsci/workflow-basic-steps-plugin#80, what I actually advocated from the start was something more like

stage('Collect frobnitzes') { // or even label('…')
  sh '''
  mvn -B -Dsome.long.option -Dsome.even.longer.option -f whatever/subdir org.whatever:frobnitz-maven-plugin:1.0.3-beta-99:collect
  '''
}

which would allow a UI to offer whatever level of detail it could comfortably display—permitting drill-down to individual steps where feasible.