-
Notifications
You must be signed in to change notification settings - Fork 24
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
add CSV-separated job run causes to RunMessage #16
base: master
Are you sure you want to change the base?
Conversation
@@ -63,6 +67,13 @@ public RunMessage(@Nonnull Run run) { | |||
set(EventProps.Jenkins.jenkins_object_url, run.getUrl()); | |||
set(EventProps.Job.job_run_queueId, Long.toString(run.getQueueId())); | |||
|
|||
// CSV list of the accumulated causes of this run | |||
final List<String> causes = new ArrayList<>(); | |||
for (Cause cause : (List<Cause>) run.getCauses()) { |
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.
Might want to wrap all of this in a check for null
on the return of run.getCauses()
.
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.
#getCauses is tagged @Nonnull
and looks safe:
public @Nonnull List<Cause> getCauses() {
CauseAction a = getAction(CauseAction.class);
if (a==null) return Collections.emptyList();
return Collections.unmodifiableList(a.getCauses());
}
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.
@tfennelly ping?
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.
So this will be added on every event, right? I'd restrict it just the run_started
event.
@tfennelly any objections to going forward with this? |
@tfennelly updated with recommended change. |
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.
Look good @jordanglassman
Would be important to test these changes in a Jenkins that has Blue Ocean installed.
Also of course ... the CI is failing there atm. |
@tfennelly I've tested this in all the ways I know how. Blue Ocean acceptance tests pass, and works with local testing. LMK what the next step for moving this ball forward is? |
Description
Adds a CSV-separated list of run cause simple class names for downstream consumers to decide if they want to consume these events or not.
Submitter checklist
Reviewer checklist