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.

Sign up for GitHub

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

Jump to bottom

Save information about child jobs in parent job #13

Closed
wants to merge 6 commits into from
Closed

Save information about child jobs in parent job #13

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
46b0391
Save information about child jobs in parent job
Jimilian Jun 27, 2017
755b480
Several improvements in BuildInfoAction
Jimilian Jun 28, 2017
05a8806
Implement double-checked locking for retriving the BuildInfoAction
Jimilian Jun 29, 2017
66aa284
Merge remote-tracking branch 'upstream/master' into child_jobs
Jimilian Jul 26, 2017
42aec90
Minor performance optimizations
Jimilian Jul 26, 2017
9b0afd5
Fix the issues mentioned in code review
Jimilian Aug 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildInfoAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package org.jenkinsci.plugins.workflow.support.steps.build;

import hudson.model.InvisibleAction;

import java.util.ArrayList;
import java.util.List;


public class BuildInfoAction extends InvisibleAction {
Copy link
Member

Choose a reason for hiding this comment

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

I see no problem with making it visible, but maybe it's outside the scope

Copy link
Author

Choose a reason for hiding this comment

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

I can put this comment to the code ;)

private final List<BuildInfo> buildInfoList = new ArrayList<>();

public BuildInfoAction(String projectName, int buildNumber) {
buildInfoList.add(new BuildInfo(projectName, buildNumber));
}

public List<BuildInfo> getBuildInfoList() {
return buildInfoList;
Copy link
Member

Choose a reason for hiding this comment

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

Wrap with unmodifyableList()

Copy link
Author

Choose a reason for hiding this comment

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

Gone.

}

public void addBuildInfo(String projectName, int buildNumber) {
buildInfoList.add(new BuildInfo(projectName, buildNumber));
Copy link
Member

Choose a reason for hiding this comment

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

It seems there may be concurrency issues with this field. I would recommend CopyOnWrite list or synchronize

Copy link
Author

Choose a reason for hiding this comment

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

CopyOnWrite looks like overkill, synchronize needs some manual work, so, I picked up ConcurrentLinkedQueue (current implementation of this action doesn't require random access).

}

static class BuildInfo {
private final String projectName;
private final int buildNumber;
Copy link
Member

Choose a reason for hiding this comment

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

Easier just to use String via Run.externalizableId.

Copy link
Author

Choose a reason for hiding this comment

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

It does not look easier for me, in this case I would need one String concatenation + String.split for every call for nothing. I now that it's tiny stuff, but even from code perspective it doesn't simplify much.


public BuildInfo(String projectName, int buildNumber) {
Copy link
Member

Choose a reason for hiding this comment

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

This approach may start leaking job existence fact if the API user has no Item.Discover permission. Not sure if it's acceptable

Copy link
Author

Choose a reason for hiding this comment

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

How can I prevent this?
Btw, is it possible that user has access to list of actions of some build, but doesn't have access to the log of this build? (right now plugin publishes names/build numbers of triggered jobs to the log).

For now I made this class private and only list of runs can be retrieved from action. I hope that it would be safer.

this.projectName = projectName;
this.buildNumber = buildNumber;
}

public String getProjectName() {
return projectName;
}

public int getBuildNumber() {
return buildNumber;
}
}
}
10 changes: 10 additions & 0 deletions src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import hudson.model.TaskListener;
import hudson.model.listeners.RunListener;
import java.util.logging.Level;
import org.jenkinsci.plugins.workflow.graph.FlowNode;
import org.jenkinsci.plugins.workflow.steps.StepContext;

import java.util.logging.Logger;
Expand All @@ -29,6 +30,15 @@ public void onStarted(Run<?, ?> run, TaskListener listener) {
TaskListener taskListener = stepContext.get(TaskListener.class);
// encodeTo(Run) calls getDisplayName, which does not include the project name.
taskListener.getLogger().println("Starting building: " + ModelHyperlinkNote.encodeTo("/" + run.getUrl(), run.getFullDisplayName()));
FlowNode parentNode = stepContext.get(FlowNode.class);
if (parentNode != null) {
Copy link
Author

Choose a reason for hiding this comment

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

@oleg-nenashev, I suppose that this code is not thread-safe as well. Is it correct?
I think to put synchronized(this) for addAction/getAction

Copy link
Member

Choose a reason for hiding this comment

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

No need to check for null if defined via @StepContextParameter (which is actually deprecated but fixing that requires a larger change).

BuildInfoAction buildInfoAction = parentNode.getAction(BuildInfoAction.class);
if (buildInfoAction == null) {
parentNode.addAction(new BuildInfoAction(run.getParent().getFullName(), run.getNumber()));
} else {
buildInfoAction.addBuildInfo(run.getParent().getFullName(), run.getNumber());
}
}
} catch (Exception e) {
LOGGER.log(WARNING, null, e);
}
Expand Down
48 changes: 42 additions & 6 deletions src/test/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerStepTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
import static org.hamcrest.Matchers.nullValue;
import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
import org.jenkinsci.plugins.workflow.cps.CpsFlowExecution;
import org.jenkinsci.plugins.workflow.graph.FlowGraphWalker;
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.test.steps.SemaphoreStep;
Expand Down Expand Up @@ -69,20 +71,30 @@ public class BuildTriggerStepTest {
us.setDefinition(new CpsFlowDefinition(
"def ds = build 'ds'\n" +
"echo \"ds.result=${ds.result} ds.number=${ds.number}\"", true));
j.assertLogContains("ds.result=SUCCESS ds.number=1", j.buildAndAssertSuccess(us));
WorkflowRun usRun = j.buildAndAssertSuccess(us);
j.assertLogContains("ds.result=SUCCESS ds.number=1", usRun);
// TODO JENKINS-28673 assert no warnings, as in StartupTest.noWarnings
// (but first need to deal with `WARNING: Failed to instantiate optional component org.jenkinsci.plugins.workflow.steps.scm.SubversionStep$DescriptorImpl; skipping`)
ds.getBuildByNumber(1).delete();

assertBuildInfoAction(usRun, "ds", 1);
}

@Issue("JENKINS-25851")
@Test public void failingBuild() throws Exception {
j.createFreeStyleProject("ds").getBuildersList().add(new FailureBuilder());
FreeStyleProject ds = j.createFreeStyleProject("ds");
ds.getBuildersList().add(new FailureBuilder());
WorkflowJob us = j.jenkins.createProject(WorkflowJob.class, "us");
us.setDefinition(new CpsFlowDefinition("build 'ds'", true));
j.assertBuildStatus(Result.FAILURE, us.scheduleBuild2(0));
WorkflowRun usRun = j.assertBuildStatus(Result.FAILURE, us.scheduleBuild2(0));
assertBuildInfoAction(usRun, "ds", ds.getLastBuild().getNumber());

us.setDefinition(new CpsFlowDefinition("echo \"ds.result=${build(job: 'ds', propagate: false).result}\"", true));
j.assertLogContains("ds.result=FAILURE", j.buildAndAssertSuccess(us));

usRun = j.buildAndAssertSuccess(us);
j.assertLogContains("ds.result=FAILURE", usRun);

assertBuildInfoAction(usRun, "ds", ds.getLastBuild().getNumber());
}

@SuppressWarnings("deprecation")
Expand Down Expand Up @@ -115,7 +127,10 @@ public void buildParallelTests() throws Exception {
" build('test2');\n" +
" })"), "\n"), true));

j.buildAndAssertSuccess(foo);
WorkflowRun usRun = j.buildAndAssertSuccess(foo);

assertBuildInfoAction(usRun, "test1", p1.getLastBuild().getNumber());
assertBuildInfoAction(usRun, "test2", p2.getLastBuild().getNumber());
}


Expand All @@ -141,7 +156,11 @@ public void abortBuild() throws Exception {
fb.getExecutor().interrupt();

j.assertBuildStatus(Result.ABORTED, j.waitForCompletion(fb));
j.assertBuildStatus(Result.FAILURE,q.get());

WorkflowRun usRun = q.get();
j.assertBuildStatus(Result.FAILURE, q.get());

assertBuildInfoAction(usRun, "test1", p.getLastBuild().getNumber());
}

@Test
Expand Down Expand Up @@ -420,4 +439,21 @@ public MultiBranchProjectFactory newInstance() {
}
}
}

private void assertBuildInfoAction(WorkflowRun workflowRun, String projectName, int buildNumber) {
FlowGraphWalker walker = new FlowGraphWalker(workflowRun.getExecution());
Copy link
Member

Choose a reason for hiding this comment

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

🐜 Deprecated, use DepthFirstScanner instead

Copy link
Author

@Jimilian Jimilian Aug 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It needs workflow-api:2.17, so, I updated it in pom.xml as well, but now it fails in verification: https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fpipeline-build-step-plugin/detail/PR-13/9/tests/
Also, I think @jglick would not be happy to introduce dependency for higher version (especially in case of test code needs it -> not production).
Also I didn't find tag @Deprecated in constructor of FlowGraphWalker class in new API.

@svanoort, @jglick, @oleg-nenashev So, what should I do?


for(FlowNode step : walker) {
BuildInfoAction buildInfoAction = step.getAction(BuildInfoAction.class);
if (buildInfoAction != null) {
for (BuildInfoAction.BuildInfo buildInfo : buildInfoAction.getBuildInfoList()) {
if (buildInfo.getBuildNumber() == buildNumber && buildInfo.getProjectName().equals(projectName)) {
return;
}
}
}
}

throw new AssertionError("Didn't find expected buildInfoAction");
}
}