Conversation
ec4350c to
9f7e52b
Compare
9f7e52b to
a95cb34
Compare
The commit message and the pull request text should be adjusted to take that in account. I have thus marked this as being in draft until I fix the texts. |
a455d7e to
9eaaf80
Compare
d1e4c3a to
ee51c9c
Compare
Instead of:
if (condition != null) {
// indented large block code
} else {
log.info("Skipped");
}
Inverse the condition check to move the logging code at the top and add
an explicit continue. This saves a level of indentation on the code
block:
if (condition == null) {
log.info("Skipped");
continue;
}
// large block code
Also adjust the logging message while at it:
vv
- The project was not trigger by some reason.
+ The project was not trigger for some reason.
^^^
Instead of invoking `completedRun.getResult()` several times, invoke it once and reuse the resust. That makes the code slightly easier to read.
e0517ad to
bee3e3b
Compare
Use case -------- My use case is a post build script triggering a job which does not need to fully complete. Since the build step can be cancelled while waiting or building, it causes the triggering build to be marked as a failure despite asking to never block. To fix that, I need the plugin to pass the result of the build step through BlockingBehaviour. That lets one define how to behave when the triggered build is NOT_BUILT (it got cancelled from the queue). In my case I need it to never block. See: https://phabricator.wikimedia.org/T352319 Solution -------- There are multiple reasons for a job to not fully complete: - it is interrupted, an InterruptedException is thrown, this is still rethrown and Jenkins will mark the build as ABORTED. - it can be cancelled from the build queue raising a CancellationException. This previously raised an AbortException which Jenkins handles by marking the build as a failure. I have changed it to a NOT_BUILT result which can be process as other results (addressing my use case to have it to never block). The Jenkins Result class ranks the results as: - SUCCESS - UNSTABLE - FAILURE - NOT_BUILT - ABORTED. The NOT_BUILT and ABORTED results are thus worse than a FAILURE and would be matched as such in BlockingBehavior mapBuildStepResult() and mapBuildResult() which both use isWorseOrEqualTo() for comparison. Add a test testCancelledFromBuildQueue() to cover the CancellationException() is caught and it results in a SUCCESS (since the test blocking behavior is to never block). The ResultConditionTest test covers that BlockingBehavior is able to map NOT_BUILD and ABORTED since it has two tests explicitly cancelling and interrupting jobs. Examples -------- When a build is ongoing and when aborting it: Waiting for the completion of downstream-project downstream-project jenkinsci#7 started. downstream-project jenkinsci#7 completed. Result was ABORTED Build step 'Trigger/call builds on other projects' marked build as failure Finished: FAILURE When it is waiting in the build queue and get cancelled: Waiting for the completion of downstream-project Not built: downstream-project has been cancelled while waiting in the queue. Build step 'Trigger/call builds on other projects' marked build as failure Finished: FAILURE
bee3e3b to
0918614
Compare
|
From https://phabricator.wikimedia.org/T352319#10288944 Not built A build which was waiting for a triggered project which never started: That one got cancelled by receiving a Build aborting An other build has: That is the caller job being aborted by Zuul (our CI system driving Jenkins), the triggered job is aborted. Fail this build step if the triggered build is worse than or equal, to which is set to |
|
Hi @jenkinsci/parameterized-trigger-plugin-developers , I wrote this change a couple years ago and we had it deployed for sometime until an update of plugin eventually replaced the forked version I had deployed. Would it be possible for one of you to have a look at it? It is hopefully thoroughly detailed. Thank you :-] |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new test relies on a fixed delay and can intermittently fail before the downstream item reaches the queue.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Updates blocking trigger handling so queued cancellations are treated as NOT_BUILT and processed through BlockingBehaviour.
Changes:
- Maps queue cancellation to
Result.NOT_BUILTinstead of failing immediately. - Adds coverage for cancelled downstream builds.
- Preserves existing handling for interrupted builds.
| File | Description |
|---|---|
TriggerBuilder.java |
Handles cancellation results through blocking behavior. |
TriggerBuilderTest.java |
Tests queued downstream cancellation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| QueueTaskFuture<FreeStyleBuild> parentBuild = triggerProject.scheduleBuild2(0); | ||
|
|
||
| parentBuild.waitForStart(); | ||
| Thread.sleep(500); |
Use case
My use case is a post build script triggering a job which does not need to fully complete. Since the build step can be cancelled while waiting or building, it causes the triggering build to be marked as a failure despite asking to never block.
To fix that, I need the plugin to pass the result of the build step through
BlockingBehaviour. That lets one define how to behave when the triggered build isNOT_BUILT(it got cancelled frmo the queue). In my case I need it to never block.See: https://phabricator.wikimedia.org/T352319
Solution
There are multiple reasons for a job to not fully complete:
it is interrupted, an InterruptedException is thrown, this is still rethrown and Jenkins will mark the build as
ABORTED.it can be cancelled from the build queue raising a
CancellationException. This previously raised anAbortExceptionwhich Jenkins handles by marking the build as a failure. I have changed it to aNOT_BUILTresult which can be process as other results (addressing my use case to have it to never block).The Jenkins Result class ranks the results as:
SUCCESSUNSTABLEFAILURENOT_BUILTABORTED.The
NOT_BUILTandABORTEDresults are thus worse than aFAILUREand would be matched as such inBlockingBehaviormapBuildStepResult()andmapBuildResult()which both useisWorseOrEqualTo()for comparison.Add a test
testCancelledFromBuildQueue()to cover theCancellationException()is caught and it results in aSUCCESS(since the test blocking behavior is to never block).The
ResultConditionTesttest covers thatBlockingBehavioris able to mapNOT_BUILDandABORTEDsince it has two tests explicitly cancelling and interrupting jobs.Examples
When a build is ongoing and when aborting it:
When it is waiting in the build queue and get cancelled:
Testing done
I have created two jobs in a Jenkins spinned up with
mvn hpi:run. I have covered the behavior for cancelled jobs with a new testTriggerBuilderTest.testCancelledFromBuildQueue().The result mapping for
ABORTEDandNOT_BUILDis already covered byResultConditionTest.Submitter checklist
handle-not_built-canceled