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

Bug/jenkins 47219 skipped parallel stage wrong status #16

Merged
merged 8 commits into from Jan 2, 2018
Merged

Bug/jenkins 47219 skipped parallel stage wrong status #16

merged 8 commits into from Jan 2, 2018

Conversation

cliffmeyers
Copy link

  • See JENKINS-47219
  • I'm entirely new to this part of the code base so I've probably made some awful mistakes here. I've done my best to call out anything I expect to be questionable via comments.
  • cc @svanoort

<groupId>org.jenkinsci.plugins</groupId>
<artifactId>pipeline-model-definition</artifactId>
<version>1.2.5</version>
<scope>test</scope>
Copy link
Author

Choose a reason for hiding this comment

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

This was added a test-scoped dependency so that I could use a declarative Jenkinsfile in the test.

pom.xml Outdated
@@ -113,6 +114,11 @@
<artifactId>pipeline-stage-step</artifactId>
<version>2.2</version>
</dependency>
<dependency>
<groupId>org.jenkinsci.plugins</groupId>
<artifactId>pipeline-stage-tags-metadata</artifactId>
Copy link
Author

Choose a reason for hiding this comment

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

This was needed to avoid hard-coding some strings related to the stage tag, which is used to determine if the stage was skipped for conditional.

Copy link
Member

Choose a reason for hiding this comment

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

Technically allowable but I'd really rather we just hardcode the string because this carries along a giant wad of dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, that one has no dependencies. Deliberately. =)

Copy link
Member

Choose a reason for hiding this comment

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

Oh you're right, I was looking at the root pom. Still I don't think it's worth taking a dependency just to carry a single string.

Copy link
Author

Choose a reason for hiding this comment

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

There are technically two string values we're pulling in here via StageStatus.getSkippedForConditional() ("SKIPPED_FOR_CONDITIONAL") and StageStatus.TAG_NAME ("STAGE_STATUS"). Not sure if that changes it at all for you, guessing probably not... I can just encapsulate those values in this plugin's own StageStatus class and add a comment referencing the other plugin.

Copy link
Author

Choose a reason for hiding this comment

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

I removed this dependency in commit 40dc7ba

pom.xml Outdated
@@ -56,7 +56,8 @@
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-job</artifactId>
<version>2.0</version>
<!-- needed to avoid ClassNotFoundException for PipelineTriggersJobProperty in tests -->
<version>2.4</version>
Copy link
Author

Choose a reason for hiding this comment

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

Trying to execute the declarative pipeline in the test would fail with CNFE specified in the comment. Checking the history, this is earliest version that introduces this class. I wanted to specify this with scope=test, but it seems that Maven 3 doesn't handle the same dep being declare with different scopes. Is there a better way to handle this?

Copy link
Member

Choose a reason for hiding this comment

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

It's okay to bump the base dependency -- people will kick up a fuss if you're pulling in something dicey

Copy link
Author

Choose a reason for hiding this comment

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

Left bumped dep version and removed comment in commit a42a59e

/**
* @author cliffmeyers
*/
public class NodeUtils {
Copy link
Author

Choose a reason for hiding this comment

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

The methods in this class are borrowed from blueocean-pipeline-api-impl PipelineNodeUtil class. Given the subtly of the logic it seems important to include.. but perhaps this should be placed in some other library?

Copy link
Member

Choose a reason for hiding this comment

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

I think you could probably just look for the TagsAction being present with the specific hardcoded string needed -- eliminating the need for this.

Copy link
Author

Choose a reason for hiding this comment

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

I simplified this as you suggested in 08b0e6c

@i386
Copy link

i386 commented Dec 19, 2017

ping @abayer you might be interested in this one.

@cliffmeyers
Copy link
Author

cliffmeyers commented Dec 20, 2017

Update: I tested this in Jenkins container and found that the NodeUtils.isStage check would return false for the parallel stage yet unfortunately return true in the unit test I wrote.

  1. In unit test, node.getAction(ThreadNameAction.class) == null returns true as the StageStep's actions are: BodyInvocationAction, LabelAction, TimingAction and TagsAction
  2. In Jenkins, node.getAction(ThreadNameAction.class) == null returns false as the StageStep's actions are: BodyInvocationAction, ParallelStepExecution$ParallelLabelAction, TimingAction and TagsAction

The ParallelStepExecution$ParallelLabelAction implements ThreadNameAction so it causes the check to fail. It's unclear to me why the two execution environments are returning a different type of label action.

I could just remove the NodeUtils.isStage check entirely and just rely on this snippet of code:

TagsAction tags = node.getAction(TagsAction.class);
return tags != null && StageStatus.getSkippedForConditional().equals(tags.getTagValue(StageStatus.TAG_NAME));

this would probably work ok but I was worried about false positives. Maybe @abayer or @svanoort can weigh in. Thanks!

@@ -242,7 +245,7 @@ public static GenericStatus computeChunkStatus2(@Nonnull WorkflowRun run,
if (exec == null) {
return null;
}
if (!NotExecutedNodeAction.isExecuted(lastNode)) {
if (!NotExecutedNodeAction.isExecuted(lastNode) || checkStageSkippedForConditional(firstNode)) {
Copy link
Member

Choose a reason for hiding this comment

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

SHOULD just be able to look for a TagsAction that is not null and matches the hardcoded skipped string.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, done in 08b0e6c


@Test
@Issue("JENKINS-47219")
public void parallelStagesOneSkipped() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Testcase looks solid to me!

Copy link
Member

@svanoort svanoort left a comment

Choose a reason for hiding this comment

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

Needs to clean up the logic for checking for a skipped stage and cut out the extra dependency on pipeline model definition metadata (a single hardcoded string is a fair trade). Other than that looks good to me.

Thanks for the contribution!

Cliff Meyers added 3 commits December 28, 2017 09:51
…n when running in unit test and ParallelStepExecution$ParallelLabelAction when running in Jenkins container is the problem
@cliffmeyers
Copy link
Author

@svanoort thanks for the feedback. I implemented the changes you requested and the test still passes. I also tested this in-container and found that the simplified check for a skipped stage works well. Hopefully this is good to go.

@svanoort
Copy link
Member

svanoort commented Jan 2, 2018

@cliffmeyers It looks good to me now, thanks for the solid contribution!

@svanoort svanoort merged commit f1b74e9 into jenkinsci:master Jan 2, 2018
@cliffmeyers
Copy link
Author

@svanoort thanks! Can you publish a new version of this lib so that blueocean can tick up its dependency on it? This will allow me to resolve the linked issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants