chore(release): factor code, make testable, clarify workflow names#3882
Conversation
Factored out cmd_* functions from release.py into separate classes with explicit dependency injection of Git and GitHub helpers. This improves testability and code organization. Simplified mocks in tests to use injection instead of module-level patching.
Renamed release-related GHA workflow files in .github/workflows to start with 'release_' and match the corresponding subcommands of the release tool. Also unified extension to .yaml.
Updated the name field in the GHA workflow files to follow the 'Release: <Command Name>' convention, matching the release tool subcommands.
![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 release tools by migrating module-level functions in git.py and gh.py into object-oriented Git and GitHub helper classes, and reorganizing subcommands into dedicated class-based command modules. The review feedback provides valuable robustness improvements, such as safely handling potentially missing or null fields in GitHub API responses (like mergeCommit and PR body), wrapping string-to-integer conversions in try-except blocks to prevent crashes on malformed references, and replacing hardcoded "upstream" remote references with a configurable --remote argument in the promote-rc subcommand.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| commit_sha = pr_info["mergeCommit"]["oid"] | ||
| short_commit = commit_sha[:8] |
There was a problem hiding this comment.
Directly accessing pr_info["mergeCommit"]["oid"] can raise a KeyError or TypeError if mergeCommit is missing or None. It is safer to retrieve it using .get() and verify its presence before accessing the OID.
| commit_sha = pr_info["mergeCommit"]["oid"] | |
| short_commit = commit_sha[:8] | |
| merge_commit = pr_info.get("mergeCommit") | |
| if not merge_commit or "oid" not in merge_commit: | |
| print(f"Error: PR #{args.pr} has no merge commit SHA.") | |
| return 1 | |
| commit_sha = merge_commit["oid"] | |
| short_commit = commit_sha[:8] |
| pr_num = int(item.pr_ref.lstrip("#")) | ||
| print(f"Resolving PR #{pr_num} to merge commit...") | ||
| try: |
There was a problem hiding this comment.
If item.pr_ref is malformed or contains non-digit characters (e.g., due to manual edits in the tracking issue), int() will raise a ValueError outside of the try block, crashing the entire command. Moving the parsing inside a dedicated try-except block ensures that any parsing errors are gracefully caught and handled as resolution failures.
| pr_num = int(item.pr_ref.lstrip("#")) | |
| print(f"Resolving PR #{pr_num} to merge commit...") | |
| try: | |
| try: | |
| pr_num = int(item.pr_ref.lstrip("#")) | |
| except ValueError: | |
| print(f"Error: Invalid PR reference '{item.pr_ref}'. Gating.") | |
| item.status = "error-resolution-failed" | |
| resolved_items.append(item) | |
| continue | |
| print(f"Resolving PR #{pr_num} to merge commit...") | |
| try: |
| return 1 | ||
|
|
||
| # Resolve issue number from PR body | ||
| pr_body = pr_info.get("body", "") |
There was a problem hiding this comment.
If the PR has an empty description, GitHub CLI returns null for the body, which parses to None in Python. Calling re.search on None will raise a TypeError. Use pr_info.get("body") or "" to safely handle this case.
| pr_body = pr_info.get("body", "") | |
| pr_body = pr_info.get("body") or "" |
| """Executes the promote-rc subcommand (Phase 3).""" | ||
| args = self.args | ||
| # Fetch from upstream to ensure we have the latest tags | ||
| self.git.fetch("upstream", tags=True, force=True) |
There was a problem hiding this comment.
Hardcoding the remote name as "upstream" can cause failures for users who have configured their main remote under a different name (e.g., "origin"). It is better to use args.remote to allow flexibility.
| self.git.fetch("upstream", tags=True, force=True) | |
| self.git.fetch(args.remote, tags=True, force=True) |
| if version is None: | ||
| version = determine_next_version() | ||
|
|
||
| latest_rc = get_latest_rc_tag(version, remote="upstream") |
There was a problem hiding this comment.
Use args.remote instead of hardcoding "upstream" to match the configured remote.
| latest_rc = get_latest_rc_tag(version, remote="upstream") | |
| latest_rc = get_latest_rc_tag(version, remote=args.remote) |
|
|
||
| # Tag the specific commit without checkout, and push to upstream | ||
| self.git.tag(version, commit_sha) | ||
| self.git.push("upstream", version) |
There was a problem hiding this comment.
Use args.remote instead of hardcoding "upstream" to push the promoted tag to the correct remote.
| self.git.push("upstream", version) | |
| self.git.push(args.remote, version) |
| parser.add_argument( | ||
| "--issue", | ||
| type=int, | ||
| help="The tracking issue number (optional).", | ||
| ) |
There was a problem hiding this comment.
Add a --remote argument to the parser (defaulting to "upstream") to allow users to specify their remote name, matching other subcommands.
| parser.add_argument( | |
| "--issue", | |
| type=int, | |
| help="The tracking issue number (optional).", | |
| ) | |
| parser.add_argument( | |
| "--issue", | |
| type=int, | |
| help="The tracking issue number (optional).", | |
| ) | |
| parser.add_argument( | |
| "--remote", | |
| type=str, | |
| default="upstream", | |
| help="The git remote to fetch from and push to (default: upstream).", | |
| ) |
1 participant
This refactors the release tool code to be more testable and broken
up into smaller files. Workflows are also renamed to better match
what they do and the command they run.