Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-55410] Added label attribute for documentation and clarity in jenkins views #93

Merged
merged 2 commits into from Jan 8, 2019

Conversation

@soenkekueper
Copy link
Contributor

soenkekueper commented Jan 4, 2019

See JENKINS-55410.

I've added an new Attribute "label" to the durable steps, so that this is displayed within the pipeline steps view and blue ocean views instead of the technical Name "Windows Batch Script" or "Linux Shell script". We have got a lot of this and it is very hard to find the right one.

With this labels this step can now display the real domain specific information for example "build manual" "copy driver files" etc. This makes the pipeline views very more comfortable to use.

…ation for the step to be displayed in the "Pipeline Steps" and Blue Ocean Views. So these views are more domain specific instead of technical. Instead of a lot of "Windows Batch Script" you can see for example "Clean up directory" "Copy files" and "Run build.cmd"
@dwnusbaum

This comment has been minimized.

Copy link
Member

dwnusbaum commented Jan 4, 2019

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?

@soenkekueper

This comment has been minimized.

Copy link
Contributor Author

soenkekueper commented Jan 4, 2019

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?

@soenkekueper soenkekueper changed the title Added label attribute for documentation and clarity in jenkins views [JENKINS-55410] Added label attribute for documentation and clarity in jenkins views Jan 5, 2019
@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jan 7, 2019

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

@soenkekueper

This comment has been minimized.

Copy link
Contributor Author

soenkekueper commented Jan 7, 2019

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

procueds this blue-ocean view
labeledsteppipeline

and this step view

labeledstepstepview

Copy link
Member

abayer left a comment

Other than the label trimming I mentioned, this seems good - the screenshots look great (thanks, btw!), and with the label trimming, this shouldn't have detrimental side effects.

soenkekueper
Using the code snippet generator will display an error message, if the value is longer than 100 characters.
If specified in a JenkinsFile the label will be trimmed to (the first) 100 characters.
@soenkekueper

This comment has been minimized.

Copy link
Contributor Author

soenkekueper commented Jan 7, 2019

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...

@dwnusbaum

This comment has been minimized.

Copy link
Member

dwnusbaum commented Jan 7, 2019

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.

@dwnusbaum dwnusbaum closed this Jan 7, 2019
@dwnusbaum dwnusbaum reopened this Jan 7, 2019
@jennbriden

This comment has been minimized.

Copy link

jennbriden commented Jan 7, 2019

LGTM (the UX PM)

@dwnusbaum dwnusbaum merged commit 20dc6a9 into jenkinsci:master Jan 8, 2019
2 checks passed
2 checks passed
continuous-integration/jenkins/incrementals Deployed to Incrementals.
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
@dwnusbaum

This comment has been minimized.

Copy link
Member

dwnusbaum commented Jan 8, 2019

Thanks for the improvement @soenkekueper!

@soenkekueper soenkekueper deleted the soenkekueper:labeled-steps branch Jan 8, 2019
@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jan 16, 2019

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

@bitwiseman

This comment has been minimized.

Copy link

bitwiseman commented Jan 23, 2019

@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.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jan 24, 2019

@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.)

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jan 24, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.