Improve reporting during consolidation - #754
Conversation
99049b9 to
1d27cef
Compare
lbarcziova
left a comment
lbarcziova
left a comment
There was a problem hiding this comment.
thank you, there are some good improvements here. I commented about things I would change to make this less spammy. @TomasKorbar could you have a look too?
| if submitted: | ||
| logger.info("Submitted consolidation job for %s/%s", package, dist_git_branch) | ||
|
|
||
| # Post a Jira comment notifying that consolidation has been triggered |
There was a problem hiding this comment.
iiuc the code here, this is run before the actual eligibility check (list_open_mrs checking >= 2 MRs). So users could often see:
"Your MR has been queued... a consolidated MR will be created automatically"
and later on:
"Fewer than 2 MRs; nothing to do."
I think we might want to skip this and comment about it just when we are 100% sure, to avoid spammy behaviour
There was a problem hiding this comment.
instead, I would prefer if we had a dedicated comment once the pre-checks are run and satisfied with something like "Consolidation is starting", wdyt?
There was a problem hiding this comment.
My ideal state, would be that no maintainer has to ask: "What is Ymir waiting for?" Obviously we can't control that, but we should at least be able to tell them: "Check Jira comments."
So I'm open to moving the triggered message to the point when all conditions are cleared, but we should notify on all cases when the consolidation is impossible.
There was a problem hiding this comment.
My ideal state, would be that no maintainer has to ask: "What is Ymir waiting for?" Obviously we can't control that, but we should at least be able to tell them: "Check Jira comments."
I totally agree on this, but this needs to be also aligned with not spamming them, which commenting if there is just 1 MR seems like to me. Wondering if we could maybe utilise labels as well?
There was a problem hiding this comment.
I think labels may be worse. There is already a lot of them, and we should make people memorize them all. How about modifying the comment instead? That wouldn't create a new comment, and it would provide all the information.
There was a problem hiding this comment.
that makes sense, but regarding the 1 MR scenario, I would still avoid commenting on that (what I described in #754 (comment))
There was a problem hiding this comment.
I would be too for removing this and the 1MR comment. Proper fix for non-transparency during job submission is to make information how do the queues work accessible in trace server.
There was a problem hiding this comment.
Ok. No comments on the consolidation trigger.
There was a problem hiding this comment.
I've removed both, if it is fine like this, I'll squash those patches so we have clean history for the merge.
| success=True, | ||
| status="Fewer than 2 unique MRs resolved; nothing to do.", | ||
| status="nothing_to_consolidate", | ||
| status_detail="Fewer than 2 unique MRs resolved; nothing to do.", |
There was a problem hiding this comment.
I feel this might be spammy as well (basically commenting additionally for all cases of just 1 PR iiuc?) -- if we have instead commenting on consolidation starting, I would suppress this
1d27cef to
2f6424e
Compare
Signed-off-by: Jiri Podivin <jpodivin@redhat.com>
…orkflow The `MRConsolidationOutputSchema` now includes fields `status` and `status_detail`. Valid values of the `status` are limited to `ConsolidationStatus` literal. The `handle_failure` step now routes to `update_jira_issues` step, only logging error and optionally filling `jira_issues_collected` if it is not set. The `update_jira_issues` returns `Workflow.END` if the status field of `MRConsolidationOutputSchema` is set to "failed", "nothing_to_consolidate" or "error". Signed-off-by: Jiri Podivin <jpodivin@redhat.com> Assisted-by: Claude Sonnet 4.5 via Claude Code
Signed-off-by: Jiri Podivin <jpodivin@redhat.com>
Signed-off-by: Jiri Podivin <jpodivin@redhat.com>
Signed-off-by: Jiri Podivin <jpodivin@redhat.com>
162bfef to
08aea2c
Compare
3 participants
Unfortunately, in order to have agent post comments on jiras consistently, I had to slightly modify the workflow. Alternatives involved more convoluted rewrites that I liked even less.
The
MRConsolidationOutputSchemanow includes fieldsstatusandstatus_detail. Valid values of thestatusare limitedto
ConsolidationStatusliteral.The
handle_failurestep now routes toupdate_jira_issuesstep, only logging error and optionally fillingjira_issues_collectedif it is not set.The
update_jira_issuesreturnsWorkflow.ENDif the status field ofMRConsolidationOutputSchemais set to "failed", "nothing_to_consolidate" or "error".There is a bit of defensive programming between lines 1671 and 1680, in cases the status is not recognized.
I'm open to removing it, since such a case should trigger a validation error. But, I would also understand keeping it in.