-
Notifications
You must be signed in to change notification settings - Fork 72
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
Store DownstreamBuildAction
on Run
instead of FlowNode
for consistent persistence even on completed builds
#128
Store DownstreamBuildAction
on Run
instead of FlowNode
for consistent persistence even on completed builds
#128
Conversation
src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerListener.java
Outdated
Show resolved
Hide resolved
… persistence even on completed builds
DownstreamBuildAction
immutable to fix persistence across restarts in some casesDownstreamBuildAction
on Run
instead of FlowNode
for consistent persistence even on completed builds
...est/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerStepRestartTest.java
Outdated
Show resolved
Hide resolved
} | ||
downstreamAction.setBuild(run); | ||
run.save(); | ||
upstream.save(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment this out and downstreamBuildActionUpstreamCompletesBeforeDownstreamStarts
will fail.
...est/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerStepRestartTest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerListener.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerListener.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerStepExecution.java
Outdated
Show resolved
Hide resolved
...est/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerStepRestartTest.java
Outdated
Show resolved
Hide resolved
...est/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerStepRestartTest.java
Outdated
Show resolved
Hide resolved
this.jobFullName = job.getFullName(); | ||
public static @NonNull DownstreamBuild getOrCreate(@NonNull Run<?, ?> run, @NonNull String flowNodeId, @NonNull Item job) { | ||
DownstreamBuildAction downstreamBuildAction; | ||
synchronized (DownstreamBuildAction.class) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary in the normal case since the relevant part of BuildTriggerStepExecution
runs synchronously on the CPS VM thread, but if there are queued triggered builds when updating Jenkins to pick up this PR or when Jenkins crashes then there is a possibility of race conditions leading to multiple instances of DownstreamBuildAction
without some kind of synchronization. We could drop the synchronization if we don't care about that case though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May as well keep it, does not add much clutter.
src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/DownstreamBuildAction.java
Outdated
Show resolved
Hide resolved
Not sure if https://github.com/jenkinsci/pipeline-build-step-plugin/pull/128/checks?check_run_id=19658148636 is related; maybe independently flaky? |
I am not sure. It passes locally but has failed twice in CI in this PR, once on Windows before the BOM update, and once on Linux after the BOM update. In both cases the problem seems to be that the downstream build never finishes (it seems to get stuck on a
|
See #127 (comment).
CC @Artmorse
Testing done
Submitter checklist