Fix issue with render ctl command not using branch environment variables
#536
Conversation
…d `execute_graphql_query` functions
WalkthroughRemoved branch normalization that previously derived a branch value in CLI command and validation code so the original branch argument is preserved. Introduced a fallback in utils to set Pre-merge checks (5 passed)✅ Passed checks (5 passed)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying infrahub-sdk-python with
|
| Latest commit: |
f176b6e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b3f9d12f.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://atg-20250909-issue-535.infrahub-sdk-python.pages.dev |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## stable #536 +/- ##
==========================================
+ Coverage 75.55% 75.59% +0.03%
==========================================
Files 100 100
Lines 8899 8901 +2
Branches 1748 1750 +2
==========================================
+ Hits 6724 6729 +5
+ Misses 1693 1690 -3
Partials 482 482
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
changelog/535.fixed.md (1)
1-1: Make the changelog explicit about precedence and affected command(s).Clarify the exact precedence and env var names to help operators understand the change at a glance.
Apply:
-Fix branch handling in `_run_transform` and `execute_graphql_query` functions. +fix(render): infrahubctl render now honors branch precedence + +`infrahubctl render` now resolves the default branch with this precedence: +1) explicit `--branch` CLI option +2) environment: `INFRAHUB_DEFAULT_BRANCH` (or `INFRAHUB_DEFAULT_BRANCH_FROM_GIT=true` to force using the current Git branch) +3) fallback: active branch from the local Git repository + +Implementation details: +- `_run_transform` no longer normalizes the branch; it forwards the provided value. +- `execute_graphql_query` falls back to `client.config.default_infrahub_branch` when no branch is provided. + +Fixes #535.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
changelog/535.fixed.md(1 hunks)infrahub_sdk/ctl/cli_commands.py(1 hunks)infrahub_sdk/ctl/utils.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
When implementing Infrahub checks, subclass InfrahubCheck and override validate(data); do not implement or rely on a check() method
Files:
infrahub_sdk/ctl/utils.pyinfrahub_sdk/ctl/cli_commands.py
infrahub_sdk/ctl/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
infrahub_sdk/ctl/**/*.py: Build CLI commands with Typer
Organize and keep all CLI commands within infrahub_sdk/ctl/
Files:
infrahub_sdk/ctl/utils.pyinfrahub_sdk/ctl/cli_commands.py
🧬 Code graph analysis (2)
infrahub_sdk/ctl/utils.py (1)
infrahub_sdk/config.py (1)
default_infrahub_branch(125-130)
infrahub_sdk/ctl/cli_commands.py (1)
infrahub_sdk/utils.py (1)
write_to_file(310-323)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: unit-tests (3.13)
🔇 Additional comments (2)
infrahub_sdk/ctl/utils.py (1)
122-124: Remove the suggested change—default_infrahub_branchis already a property
default_infrahub_branchis decorated with@propertyininfrahub_sdk/config.py:125, soclient.config.default_infrahub_branchreturns a string. No parentheses needed.Likely an incorrect or invalid review comment.
infrahub_sdk/ctl/cli_commands.py (1)
49-49: LGTM on import change.Import path for
write_to_fileis correct and used consistently below.
…agement in `_run_transform` and `execute_graphql_query` functions.
…_graphql` function
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/ctl/test_render_app.py (2)
78-85: Broaden precedence coverage with a few extra casesGreat start covering CLI-only, ENV-only, and Git-only. Consider adding mixed-signal cases and the config fallback to fully pin precedence.
@pytest.mark.parametrize( "cli_branch,env_branch,from_git,expected_branch", [ ("cli-branch", None, False, "cli-branch"), (None, "env-branch", False, "env-branch"), (None, None, True, "git-branch"), + # CLI should beat ENV and GIT + ("cli-branch", "env-branch", True, "cli-branch"), + # ENV should beat GIT even if GIT lookup is enabled + (None, "env-branch", True, "env-branch"), + # Fallback to client.config.default_infrahub_branch + (None, None, False, "main"), ], )
88-116: Make tests immune to host environment leakageExplicitly clear branch-related env vars before setting per-case values to avoid CI/env flakiness.
def test_render_branch_selection(monkeypatch, httpx_mock: HTTPXMock, cli_branch, env_branch, from_git, expected_branch): """Test that the render command uses the correct branch source.""" + # Ensure deterministic environment for each parametrized run + monkeypatch.delenv("INFRAHUB_DEFAULT_BRANCH", raising=False) + monkeypatch.delenv("INFRAHUB_DEFAULT_BRANCH_FROM_GIT", raising=False) + if from_git: monkeypatch.setattr("dulwich.porcelain.active_branch", lambda _: b"git-branch")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
changelog/535.fixed.md(1 hunks)tests/fixtures/repos/ctl_integration/.infrahub.yml(1 hunks)tests/fixtures/repos/ctl_integration/templates/tags.j2(1 hunks)tests/fixtures/repos/ctl_integration/templates/tags_query.gql(1 hunks)tests/unit/ctl/test_render_app.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/fixtures/repos/ctl_integration/templates/tags_query.gql
- tests/fixtures/repos/ctl_integration/templates/tags.j2
🚧 Files skipped from review as they are similar to previous changes (1)
- changelog/535.fixed.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
When implementing Infrahub checks, subclass InfrahubCheck and override validate(data); do not implement or rely on a check() method
Files:
tests/unit/ctl/test_render_app.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place and write unit tests under tests/unit/ (isolated component tests)
Files:
tests/unit/ctl/test_render_app.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use the custom pytest plugin (infrahub_sdk.pytest_plugin) fixtures for clients, configuration, and Infrahub-specific testing
Files:
tests/unit/ctl/test_render_app.py
🧬 Code graph analysis (1)
tests/unit/ctl/test_render_app.py (3)
tests/helpers/fixtures.py (1)
read_fixture(10-16)tests/helpers/utils.py (1)
temp_repo_and_cd(31-42)infrahub_sdk/async_typer.py (1)
runner(17-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: unit-tests (3.13)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
tests/fixtures/repos/ctl_integration/.infrahub.yml (3)
29-32: LGTM: transform wiring is correctThe jinja2 transform correctly references the query and template path. This aligns with the new render test.
37-38: LGTM: query registrationRegistering tags_query under queries is consistent with the transform usage.
29-38: Fixture files verified
Both tags.j2 and tags_query.gql are present under tests/fixtures/repos/ctl_integration/templates.tests/unit/ctl/test_render_app.py (2)
90-92: Mock return type matches dulwich (bytes) — goodReturning bytes mirrors dulwich.porcelain.active_branch; keeps the behavior realistic.
104-116: LGTM: end-to-end assertion via URL matchingUsing the branch-specific GraphQL path ensures the selected source is exercised without asserting internals.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
infrahub_sdk/ctl/validate.py (3)
77-79: Log the resolved branch (avoid printing 'None')You print before resolving the fallback, so users may see
branch 'None'. Move this print after the fallback or compute the resolved value first.Example (outside changed lines):
# after computing branch fallback console.print(f"[purple]Query '{query}' will be validated on branch '{branch}'.")
65-66: Type hint should be Optional[str]The option defaults to None; reflect that in the annotation.
- branch: str = typer.Option(None, help="Branch on which to validate the GraphQL Query."), + branch: Optional[str] = typer.Option(None, help="Branch on which to validate the GraphQL Query."),
101-102: Improve guidance to mention env-based fallbackSince fallback honors env, consider mentioning INFRAHUB_DEFAULT_BRANCH and INFRAHUB_DEFAULT_BRANCH_FROM_GIT for discoverability.
- console.print("[yellow] you can specify a different branch with --branch") + console.print("[yellow] you can specify a different branch with --branch or set INFRAHUB_DEFAULT_BRANCH / INFRAHUB_DEFAULT_BRANCH_FROM_GIT")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrahub_sdk/ctl/validate.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
When implementing Infrahub checks, subclass InfrahubCheck and override validate(data); do not implement or rely on a check() method
Files:
infrahub_sdk/ctl/validate.py
infrahub_sdk/ctl/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
infrahub_sdk/ctl/**/*.py: Build CLI commands with Typer
Organize and keep all CLI commands within infrahub_sdk/ctl/
Files:
infrahub_sdk/ctl/validate.py
🧬 Code graph analysis (1)
infrahub_sdk/ctl/validate.py (2)
infrahub_sdk/utils.py (1)
write_to_file(310-323)infrahub_sdk/config.py (1)
default_infrahub_branch(125-130)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: unit-tests (3.13)
🔇 Additional comments (2)
infrahub_sdk/ctl/validate.py (2)
17-17: LGTM on import usage
write_to_fileis correctly imported and used later for--out.
83-85: No change needed:default_infrahub_branchis a property
default_infrahub_branchis decorated with@propertyininfrahub_sdk/config.pyand returns astr, so using it without parentheses is correct.Likely an incorrect or invalid review comment.
Fixes: #535
Summary by CodeRabbit