Conversation
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🔴 👀 Review RequirementsThis rule is failing.
🔴 🔎 ReviewsThis rule is failing.
🟢 🤖 Continuous IntegrationWonderful, this rule succeeded.
🟢 Enforce conventional commitWonderful, this rule succeeded.Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
🟢 📕 PR descriptionWonderful, this rule succeeded.
|
🧪 CI InsightsHere's what we observed from your CI run for 389e2f9. 🟢 All jobs passed!But CI Insights is watching 👀 |
572ba4a to
e5d5b20
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new mergify freeze command group to manage scheduled merge freezes via the Mergify API, along with a dedicated API wrapper and CLI tests.
Changes:
- Introduces
freezeCLI group withlist,create,update, anddeletesubcommands, including table/JSON output. - Adds
mergify_cli.freeze.apiclient functions for the scheduled freeze endpoints. - Adds a comprehensive
respx-based CLI test suite for the new command group.
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
mergify_cli/freeze/cli.py |
Implements the freeze click command group, formatting/output, and default token/repo detection. |
mergify_cli/freeze/api.py |
Adds async API calls for list/create/update/delete scheduled freezes. |
mergify_cli/cli.py |
Registers the new freeze command group in the top-level CLI. |
mergify_cli/tests/freeze/test_cli.py |
Adds CLI tests covering list/create/update/delete flows and error paths. |
mergify_cli/tests/freeze/__init__.py |
Adds package marker for freeze tests. |
mergify_cli/freeze/__init__.py |
Adds package marker for freeze module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e5d5b20 to
097f802
Compare
097f802 to
9ee9a71
Compare
Add `mergify freeze` command group with list, create, update, and delete subcommands for managing scheduled merge freezes. Supports both Mergify application keys and GitHub tokens for authentication, auto-detects repository from git remote, and provides both table and JSON output. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Change-Id: I414d168066f7b5070e75520bdcbb8189d694ad72 Claude-Session-Id: 77d8be2d-854a-4d80-ac2d-4489fa50e31d
9ee9a71 to
389e2f9
Compare
There was a problem hiding this comment.
No error handling on API responses
The API functions (list_freezes, create_freeze, etc.) call .json() on responses without checking status codes first. The check_for_status event hook on the HTTP client will raise on 4xx/5xx, but only after
aread() — if the hook fails to trigger or the client is used differently, you'd get silent failures. This is consistent with the existing codebase pattern though, so it's acceptable.
| required=True, | ||
| help="Matching condition (repeatable, e.g. -c 'base=main')", | ||
| ) | ||
| @click.option( |
There was a problem hiding this comment.
Users shouldn't need to re-specify every field just to change the end time ?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except CommandError: | ||
| return None | ||
|
|
||
| try: | ||
| user, repo_name = get_slug(remote_url) | ||
| except (ValueError, IndexError): | ||
| return None | ||
|
|
There was a problem hiding this comment.
New auto-detection logic in get_default_repository() is only tested for the GITHUB_REPOSITORY env-var path. Consider adding tests for the git-remote path (e.g., using the existing git_mock fixture that mocks remote.origin.url) and for the failure case to ensure the CLI surfaces a clear error when repository detection isn't possible.
| except CommandError: | |
| return None | |
| try: | |
| user, repo_name = get_slug(remote_url) | |
| except (ValueError, IndexError): | |
| return None | |
| except CommandError: | |
| console.print( | |
| "error: unable to detect the default repository. " | |
| "Please set the 'GITHUB_REPOSITORY' environment variable or " | |
| "run this command from a Git repository with a configured " | |
| "'origin' remote.", | |
| style="red", | |
| ) | |
| return None | |
| try: | |
| user, repo_name = get_slug(remote_url) | |
| except (ValueError, IndexError): | |
| console.print( | |
| "error: unable to parse the repository from the git remote URL. " | |
| "Please set the 'GITHUB_REPOSITORY' environment variable.", | |
| style="red", | |
| ) | |
| return None | |
| if not user or not repo_name: | |
| console.print( | |
| "error: detected an invalid repository slug. " | |
| "Please set the 'GITHUB_REPOSITORY' environment variable.", | |
| style="red", | |
| ) | |
| return None |
| async def get_default_token() -> str | None: | ||
| token = os.environ.get("MERGIFY_TOKEN") or os.environ.get("GITHUB_TOKEN") | ||
| if not token: | ||
| try: | ||
| token = await run_command("gh", "auth", "token") | ||
| except CommandError: | ||
| console.print( | ||
| "error: please set the 'MERGIFY_TOKEN' or 'GITHUB_TOKEN' environment variable, " | ||
| "or make sure that gh client is installed and you are authenticated", | ||
| style="red", | ||
| ) | ||
| return None | ||
| return token |
There was a problem hiding this comment.
get_default_token() returns None on failure but callers (e.g., get_mergify_http_client(api_url, token)) treat token as a required string and will happily send Authorization: Bearer None or trigger Click's MissingParameter after already printing an error. Consider making this helper fail fast by raising a click.ClickException/SystemExit (or returning an empty string consistently and letting the Click option validation handle it), rather than returning None.
| async def get_default_repository() -> str | None: | ||
| repo = os.environ.get("GITHUB_REPOSITORY") | ||
| if repo: | ||
| return repo | ||
|
|
||
| try: | ||
| remote_url = await git( | ||
| "config", | ||
| "--get", | ||
| "remote.origin.url", | ||
| ) | ||
| except CommandError: | ||
| return None | ||
|
|
||
| try: | ||
| user, repo_name = get_slug(remote_url) | ||
| except (ValueError, IndexError): | ||
| return None | ||
|
|
||
| return f"{user}/{repo_name}" |
There was a problem hiding this comment.
get_default_repository() silently returns None when it can't detect the repo (no GITHUB_REPOSITORY, no remote.origin.url, or unparsable URL). Since the Click option is marked required=True, this turns into a generic "Missing option" error with no actionable guidance. Consider printing/raising a clear error explaining the supported detection methods (env var, --repository, or git remote) when detection fails.
| "-t", | ||
| help="Mergify or GitHub token", | ||
| envvar=["MERGIFY_TOKEN", "GITHUB_TOKEN"], | ||
| required=True, |
There was a problem hiding this comment.
At the command-group level, --token is marked required=True but its default can evaluate to None (see utils.get_default_token()), which causes Click to raise MissingParameter after already printing a custom error message. This also makes mergify freeze (with no subcommand) prone to failing even though the handler tries to print help. Consider removing required=True on the group option and instead validating in freeze() only when a subcommand is invoked (or make the default raise a Click exception directly).
| required=True, |
| @click.option( | ||
| "--repository", | ||
| "-r", | ||
| help="Repository full name (owner/repo)", | ||
| required=True, | ||
| default=lambda: asyncio.run(utils.get_default_repository()), | ||
| ) |
There was a problem hiding this comment.
Same issue as --token: --repository is required=True but the default detection can return None, which will prevent mergify freeze from showing help and yields a generic missing-parameter error without guidance. Consider making the group option non-required and performing explicit validation only when a subcommand runs, with a clear error message when auto-detection fails.
Add
mergify freezecommand group with list, create, update, and deletesubcommands for managing scheduled merge freezes. Supports both Mergify
application keys and GitHub tokens for authentication, auto-detects
repository from git remote, and provides both table and JSON output.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com