Conversation
…efs and scopes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Change-Id: I0b7bb94a7e7aff249ee90b650392dbae68f370fe
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 084364b. 🟢 All jobs passed!But CI Insights is watching 👀 |
There was a problem hiding this comment.
Pull request overview
Adds support for selecting the Mergify YAML config via CLI/environment for CI scope detection, with a regression test around MERGIFY_CONFIG_PATH behavior.
Changes:
- Adds
MERGIFY_CONFIG_PATHas an envvar source for theci scopes --configoption and tightens path validation. - Adds a test asserting that an empty
MERGIFY_CONFIG_PATHshould fall back to config auto-detection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
mergify_cli/ci/cli.py |
Updates the scopes command --config Click option to read from MERGIFY_CONFIG_PATH and require an existing file. |
mergify_cli/tests/ci/test_cli.py |
Adds a test for “empty env var => autodetect config” behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @click.option( | ||
| "--config", | ||
| "config_path", | ||
| type=click.Path(), | ||
| type=click.Path(exists=True, dir_okay=False), | ||
| envvar="MERGIFY_CONFIG_PATH", | ||
| default=detector.get_mergify_config_path, | ||
| help="Path to YAML config file.", |
There was a problem hiding this comment.
MERGIFY_CONFIG_PATH set to an empty string will be treated by Click as an explicit option value, and with click.Path(exists=True, dir_okay=False) it will fail parameter validation (exit code 2) before default=get_mergify_config_path can run. This prevents the intended “empty env var => autodetect” behavior (and makes the new test fail). Consider either removing exists=True and validating existence inside scopes() only when a non-empty path is provided, or adding logic to treat an empty envvar value as unset before Click performs path validation.
| @click.option( | ||
| "--config", | ||
| "config_path", | ||
| type=click.Path(), | ||
| type=click.Path(exists=True, dir_okay=False), | ||
| envvar="MERGIFY_CONFIG_PATH", | ||
| default=detector.get_mergify_config_path, | ||
| help="Path to YAML config file.", |
There was a problem hiding this comment.
The PR title mentions adding a --config option / MERGIFY_CONFIG_PATH support to both git-refs and scopes, but this diff only updates the scopes command option definition. If git-refs is also meant to accept --config/envvar, that change appears to be missing; otherwise the PR title should be adjusted to match the actual change.
| monkeypatch.chdir(tmp_path) | ||
| monkeypatch.setenv("MERGIFY_CONFIG_PATH", "") | ||
|
|
||
| runner = testing.CliRunner() | ||
| result = runner.invoke(ci_cli.scopes, ["--base", "old", "--head", "new"]) | ||
|
|
||
| # The command found the auto-detected config and ran (source is manual so exit 1) | ||
| assert result.exit_code == 1 | ||
| assert "source `manual` has been set" in result.output |
There was a problem hiding this comment.
This test expects MERGIFY_CONFIG_PATH="" to fall back to config auto-detection and reach the ScopesError path (exit code 1). With the current Click option type set to click.Path(exists=True, dir_okay=False), an empty envvar value is likely to trigger Click’s parameter validation error instead (exit code 2) before the command runs. Either adjust the CLI option handling to treat empty envvar as unset, or change the test to match the actual behavior.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com