Do not needlessly create Z-Stream branch#543
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the branch mapping logic in ymir/agents/triage_agent.py to handle older Z-Streams and the latest/upcoming Z-Streams differently. While older Z-Streams are mapped directly to internal RHEL branches, the latest Z-Streams query available branches and fall back to CentOS Stream if the expected branch is missing or if an error occurs. The review feedback highlights a risk in catching generic exceptions during this check, as transient network or gateway failures would cause an incorrect silent fallback to CentOS Stream instead of triggering a retry. It is recommended to let these exceptions propagate to allow the task to be retried.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the branch mapping logic in ymir/agents/triage_agent.py to handle older Z-Streams and current/upcoming Z-Streams separately. For current Z-Streams, it queries available internal RHEL branches and falls back to CentOS Stream if the expected branch is not found. The reviewer noted that the removal of the try-except block around the external tool call could lead to unhandled exceptions crashing the workflow, and suggested restoring the error handling to ensure robustness.
| # For Z-stream bugs, always use internal RHEL branch | ||
| # Check if branch exists, but use it anyway since it will be created later if needed | ||
| if is_zstream or older_zstream: | ||
| # For older Z-Streams, always use internal RHEL branch (it must already exist) |
There was a problem hiding this comment.
The assumption that for older z-streams the branch already exist is incorrect. while in majority of cases it should hold true, sometime it is not the case for some packages and the branch has to be created. In this context tho i guess it should be handled elsewhere eg if triage agent is trying to determine patch applicability and the branch does not exist then this should be passed down to backport agent as valid backport target ie the patch we're trying to backport is definitely not in the target branch so it should be backported (and the related branch created). of course the ideal solution would be to look at the previous branch (the new one will be created from) to check for eligibility.
There was a problem hiding this comment.
But the code is fine, only the comment is inaccurate, right?
There was a problem hiding this comment.
@nforro the code is fine up to the point where you're no longer calling mcp to get existing branches and returning before that so like i've mentioned it can have implications on triage agent decision to backport or not (i dunno if thats the case tho).
There was a problem hiding this comment.
@nforro no, i think its fine, it doesnt really change the behavior, just me being slow today
Signed-off-by: Nikola Forró <nforro@redhat.com> Assisted-by: Claude Opus 4.6 via Claude Code
antbob
left a comment
antbob
left a comment
There was a problem hiding this comment.
just some comments on older z-stream branches
3 participants
No description provided.