-
Notifications
You must be signed in to change notification settings - Fork 22
Stage all patch files prefixed with Jira issue title during backport #352
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
Stage all patch files prefixed with Jira issue title during backport #352
Conversation
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.
Code Review
This pull request aims to fix an issue in the git am workflow where patch files with suffixes (e.g., -0) were not being staged. The change correctly uses a glob pattern to find all patch files prefixed with the Jira issue key. This is a good fix that addresses the problem.
I've added one high-severity comment regarding the fallback logic. The current implementation can lead to silent failures if the fallback patch file doesn't exist. My suggestion is to add an explicit check for the file's existence and raise an exception if it's not found, ensuring the workflow halts correctly instead of proceeding with an incomplete set of changes.
Without this, the git am workfow doesn't work, as the patch file always starts with -0 suffix.
cc0c611 to
3b36789
Compare
|
/gemini review |
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.
Code Review
This pull request updates the backport agent to correctly stage all patch files associated with a Jira issue. The previous implementation used a hardcoded patch file name, which would fail for workflows that generate multiple patch files with suffixes (e.g., -0, -1). The new implementation uses pathlib.Path.glob to find all files matching the pattern {jira_issue}*.patch, ensuring that all relevant patches are staged for commit. The change is correct, well-implemented, and improves the robustness of the backporting process.
| async def stage_changes(state): | ||
| try: | ||
| # Find all patch files matching the jira issue pattern | ||
| patch_files = list(state.local_clone.glob(f"{state.jira_issue}*.patch")) |
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.
I would maybe use a bit stricter pattern, to match what's in the instructions:
| patch_files = list(state.local_clone.glob(f"{state.jira_issue}*.patch")) | |
| patch_files = list(state.local_clone.glob(f"{state.jira_issue}-*.patch")) |
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.
but for cherry-pick workflow, it doesn't seem to add the suffix (example). We might unify it later, but for fixing the git am workflow, I am going to merge this.
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.
Oh, right, sorry, my bad.
Without this, the git am workfow doesn't work, as the patch file always starts with -0 suffix.
Example: https://gitlab.com/redhat/centos-stream/rpms/apr-util/-/merge_requests/17/diffs