[WIP] Trying to improve the naming conventions of the backport agent#507
[WIP] Trying to improve the naming conventions of the backport agent#507mruprich wants to merge 1 commit into
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 updates the documentation in SKILL.md to enforce descriptive naming conventions for patch files in dist-git, replacing the previous Jira-issue-based naming. The review identifies critical discrepancies between these new documentation requirements and the existing implementation in the backport agent's Python code, which must be updated to ensure the agent correctly handles the new naming scheme. Additionally, the reviewer points out limitations in the GetPackageInfoTool regarding patch numbering and suggests improving the clarity of path examples in the documentation.
|
|
||
| ### Patch File Naming | ||
|
|
||
| All patch files in dist-git (and pre-downloaded upstream patches from Step 2) must use descriptive names — not Jira issue keys. Follow these rules: |
There was a problem hiding this comment.
The instructions in this file are being updated to use descriptive patch names, but the actual instructions used by the agent are currently hardcoded as strings in ymir/agents/backport_agent.py (see BACKPORT_INSTRUCTIONS and BACKPORT_INSTRUCTIONS_ZSTREAM). Since the agent does not read SKILL.md directly, these changes will not take effect until the Python file is also updated to reflect these new rules.
| 4. Run `<pkg_tool> --name={{package}} --namespace=rpms --release={{dist_git_branch}} prep` to unpack sources. | ||
| 5. Determine `unpacked_sources` — the path to the unpacked source directory tree. This is typically `<local_clone>/<name>-<version>-build/<buildsubdir>` (RPM 4.20+) or `<local_clone>/<buildsubdir>` (older RPM). Read the spec file to find `%{name}`, `%{version}`, and `%{buildsubdir}`. | ||
| 6. Download each upstream patch URL to a local file named `{{jira_issue}}-<N>.patch` (0-indexed) using `get_patch_from_url`, and save the content to `<local_clone>/{{jira_issue}}-<N>.patch`. | ||
| 6. Download each upstream patch URL using `get_patch_from_url`. Save each file under `<local_clone>/` with a **descriptive** name per **Patch File Naming** above (inspect the patch diff or commit message to choose the suffix). Do not use `{{jira_issue}}-<N>.patch` or other issue-key-based names. If multiple upstream URLs are provided, give each download its own descriptive filename. |
There was a problem hiding this comment.
This instruction tells the agent that pre-downloaded patches have descriptive names. However, the Python implementation in ymir/agents/backport_agent.py (line 934) still downloads and saves patches using the {{jira_issue}}-{idx}.patch format. This discrepancy will cause the agent to fail when it tries to locate the pre-downloaded patches using the new naming convention. The Python code in the fork_and_prepare_dist_git step must be updated to use descriptive names as well.
|
|
||
| 1. **Descriptive filenames**: Hyphen-separated words, prefixed with the package name (`{{package}}`). The name should describe the fix (from the diff, commit message, or upstream change), not the Jira key. | ||
| 2. **Single-file patches**: When a patch modifies only one source file, use that file's base name as the second field after the package prefix (e.g., `openssl-configure.ac-fix-dependency-check.patch`). | ||
| 3. **`PatchN` numbering in the spec**: Assign the next sequential `Patch` number after existing entries. Use `get_package_info` to find the highest numbered patch; numbering starts at `Patch0` and increases (`Patch1`, `Patch2`, …, `Patch25`, etc.). |
There was a problem hiding this comment.
The instruction to use get_package_info to find the highest numbered patch is problematic. The GetPackageInfoTool (defined in ymir/tools/unprivileged/specfile.py) returns a list of patch filenames but does not include the actual patch numbers from the spec file. If a package uses non-sequential numbering or starts at a high number (e.g., Patch100), the agent will be unable to determine the correct next number from the tool's output alone. Consider updating the tool to return a mapping of numbers to filenames or instructing the agent to inspect the spec file directly using the view tool.
| directory (the dist-git repository root) | ||
| (e.g., if JIRA is RHEL-114639, use /path/to/distgit/RHEL-114639.patch) | ||
| * patch_file_path: `<LOCAL_CLONE>/<NEW_PATCH_FILE>` | ||
| (e.g., `openssl-configure.ac-fix-dependency-check.patch`) |
There was a problem hiding this comment.
The example should include the <LOCAL_CLONE>/ path prefix as specified in the instruction on the previous line. This helps the agent understand it should use the full path to the patch file within the repository.
| (e.g., `openssl-configure.ac-fix-dependency-check.patch`) | |
| (e.g., /path/to/clone/openssl-configure.ac-fix-dependency-check.patch) |
| directory (the dist-git repository root) | ||
| (e.g., if JIRA is RHEL-114639, use /path/to/distgit/RHEL-114639.patch) | ||
| * patch_file_path: `<LOCAL_CLONE>/<NEW_PATCH_FILE>` | ||
| (e.g., `openssl-fix-CVE-2007-0123.patch`) |
There was a problem hiding this comment.
The example should include the <LOCAL_CLONE>/ path prefix as specified in the instruction on the previous line to ensure the agent uses the correct absolute or relative path.
| (e.g., `openssl-fix-CVE-2007-0123.patch`) | |
| (e.g., /path/to/clone/openssl-fix-CVE-2007-0123.patch) |
TomasTomecek
left a comment
TomasTomecek
left a comment
There was a problem hiding this comment.
This is really smart, great idea to not hardcode those patch names as we used to have this.
2 participants
TODO:
packit/packit.dev.As a source of the rules I used the https://fedoraproject.org/wiki/PeterGordon/SpecFormattingGuidelines#Patching guide which seems quite reasonable to me and I am actually using something similar in my packages. I think that any naming convention that uses something descriptive is better than just plain RHEL-123456.