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-26156] BodyInvoker.displayName was broken #3

Merged

Conversation

Projects
None yet
6 participants
@jglick
Copy link
Member

commented Apr 8, 2016

JENKINS-26156

Before:

  • if you passed null, or simply did not call it at all, there were body block nodes contrary to documentation
  • if you passed non-null, you would get an IllegalStateException (!)

After:

  • body block nodes are created unconditionally
  • you may call it, with a non-null name, if you want a label on the body block

@kohsuke seems to have been trying to elide body block nodes when they contribute no interesting information, as in most steps accepting a closure (node, say, as opposed to retry which might have something interesting to show like a retry count). But this was never actually implemented, and seems too dangerous to try anyway. What if a step asked to elide the body block nodes but then actually called its body twice? We would get a confused flow graph.

I did clean up the log display a bit, from what @amuniz did in jenkinsci/pipeline-plugin#215 (cf. JENKINS-31595). Before:

[Pipeline] Some display name intended for GUI : Start
[Pipeline] theActualStepName {
body text…
[Pipeline] } //theActualStepName
[Pipeline] Some display name intended for GUI : End

After:

[Pipeline] theActualStepName {
[Pipeline] {
body text…
[Pipeline] }
[Pipeline] } // theActualStepName

With some more work in WorkflowRun.logNodeMessage this could probably be improved further, to check for BodyInvocationAction and LabelAction on BlockStartNodes, giving us a more streamlined

[Pipeline] theActualStepName {
body text…
[Pipeline] } // theActualStepName

plus some variants for bodies which are actually run >1 time and/or specify a label, but I will leave that for another day.

@reviewbybees

jglick added some commits Apr 8, 2016

[JENKINS-30088] Cleaner appearance of nodes with blocks.
· Use the function name, not step display name, on the overall start and end nodes.
· Omit any text on the block start and end nodes.
@reviewbybees

This comment has been minimized.

Copy link

commented Apr 8, 2016

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.

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Apr 8, 2016

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

@jglick

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2016

Note that there was no need to touch ParallelStepExecution—it does call withStartAction(Action), which is not currently exposed as an API in BodyInvoker for use from other plugins, but the action it attaches is not only LabelAction but a ThreadNameAction. That is why this step successfully attached labels to its body invocations from the start: it bypassed the buggy code by using a plugin-private method.

@jglick jglick referenced this pull request Apr 11, 2016

Merged

Load step flow graph fix #5

private final boolean createBodyBlockNode;

public CpsBodyExecution(CpsStepContext context, List<BodyExecutionCallback> callbacks, boolean createBodyBlockNode) {
CpsBodyExecution(CpsStepContext context, List<BodyExecutionCallback> callbacks) {

This comment has been minimized.

Copy link
@amuniz

amuniz Apr 12, 2016

Member

Backward incompatible?

This comment has been minimized.

Copy link
@jglick

jglick Apr 12, 2016

Author Member

No. The class as a whole was not public, so no one could have been calling this constructor from outside anyway. (Not that we would want to support them if they did.)

This comment has been minimized.

Copy link
@amuniz

amuniz Apr 12, 2016

Member

Oh, right, I missed that.

@amuniz

This comment has been minimized.

Copy link
Member

commented Apr 12, 2016

That nested { and } confused me. Why not this:

[Pipeline] theActualStepName
[Pipeline] {
body text…
[Pipeline] }
[Pipeline] // theActualStepName

Other than that 🐝

@amuniz

This comment has been minimized.

Copy link
Member

commented Apr 12, 2016

🐝 Thanks!

@recena

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2016

@amuniz Obviously, this is only personal likes, but I prefer:

[Pipeline] theActualStepName
[Pipeline] {
body text…
[Pipeline] } // theActualStepName

it is only a note.

try {
FlowHead head = CpsThread.current().head;

StepEndNode end = new StepEndNode(head.getExecution(),

This comment has been minimized.

Copy link
@recena

recena Apr 12, 2016

Contributor

Why not one line? 😄

This comment has been minimized.

Copy link
@jglick

jglick Apr 12, 2016

Author Member

Ask whoever wrote the code originally, @kohsuke I suppose. I was merely deleting an if-block.

@recena

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2016

LGTM 🐝 though this part of Pipeline is completely new to me (no experience). I've tried the PR and does what is described in the PR.

@jglick

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2016

[Pipeline] } // theActualStepName

Not possible without much bigger changes: these are two FlowNodes, not one.

@jglick

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2016

I've tried the PR and does what is described in the PR.

Well the change to the build log was really just a side change. The main point was a fixed API, which is used by downstream PRs.

@jglick jglick merged commit 5e8cbc2 into jenkinsci:master Apr 12, 2016

1 check passed

Jenkins This pull request looks good
Details

@jglick jglick deleted the jglick:JENKINS-26156-BodyInvoker.withDisplayName branch Apr 12, 2016

@svanoort

This comment has been minimized.

Copy link
Member

commented Apr 12, 2016

🐝 I think, though I'm not 100% sure I understand all of this

@recena

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2016

@jglick In fact, I have several of his PR in my local env

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