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-37324] Step descriptions proof of concept #39

Merged
merged 13 commits into from May 24, 2017

Conversation

svanoort
Copy link
Member

@svanoort svanoort commented May 2, 2017

Part of JENKINS-37324 and downstream of jenkinsci/workflow-cps-plugin#98 plus its upstream dependencies.

TODO:

  • Add sanitization of step description strings Automatically handled by Handlebars escaping of content.

@svanoort svanoort changed the title Step descriptions proof of concept Step descriptions proof of concept [JENKINS-37324] May 2, 2017
Copy link
Member

@stephenc stephenc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AIUI

}

@JsonInclude(JsonInclude.Include.NON_NULL)
public String getParameterDescription() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be slightly less confused if plural was used, e.g. getParametersDescription

@jglick jglick changed the title Step descriptions proof of concept [JENKINS-37324] [JENKINS-37324] Step descriptions proof of concept May 2, 2017
import org.jenkinsci.plugins.workflow.actions.TimingAction;
import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
import org.jenkinsci.plugins.workflow.graph.FlowNode;
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
import org.jenkinsci.plugins.workflow.job.WorkflowRun;
import org.jenkinsci.plugins.workflow.support.steps.StageStep;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated

@@ -5,6 +5,9 @@
<div class="node-log-frame {{this.status}}" cbwf-controller="node-log" objectUrl="{{this._links.log.href}}">
<div class="node-name"><span class="glyphicon glyphicon-collapse-down" title="Expand"></span><span class="glyphicon glyphicon-collapse-up" title="Collapse"></span>
{{this.name}} {{#if this.durationMillis}}(self time {{formatTime this.durationMillis}}){{/if}}
{{#if this.parameterDescription}}
- {{this.parameterDescription}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might look better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, good point. I switched to

"name -- argumentsString -- (self time 10 ms)"
I.e.
"Shell Step -- rm -rf / -- (self time 10 hours)"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually was referring to an m-dash , not a pair of ASCII hyphens --, but whatever. :-)

@svanoort
Copy link
Member Author

@reviewbybees

@bitwiseman
Copy link

@svanoort I know this a POC, but are there any tests?

@ghost
Copy link

ghost commented May 23, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@svanoort
Copy link
Member Author

@bitwiseman Good question. Existing unit tests verify if this breaks anything for us - the net-new stuff is just getters/setters & a template operation -- not really any logic there to test.

I did manual testing to verify this works correctly in cases where we both do and do not have ArgumentsActions attached for steps, so am satisfied it works in integration.

@svanoort svanoort merged commit 29b14fc into master May 24, 2017
@svanoort svanoort deleted the step-descriptions-poc branch May 24, 2017 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants