Add infrahubctl branch report command
#637
Conversation
Deploying infrahub-sdk-python with
|
| Latest commit: |
c850da3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a2000a23.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://wvd-20251114-infp388-branch.infrahub-sdk-python.pages.dev |
WalkthroughAdds diff tree support: introduces a new Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/ci.yml (2)
123-160: Override working directory for uv commands in docs jobThe docs job sets
defaults.run.working-directory: ./docs, butpyproject.tomlanduv.lockexist only at the repo root. Runninguv syncanduv runfrom./docswill fail because uv cannot find the project configuration.Override the working directory for these steps to execute from the repo root:
- name: Install dependencies working-directory: . run: uv sync --all-groups --all-extras - name: "Build docs website" working-directory: . run: "uv run invoke docs"
213-266: Add explicitpython-versioninput toastral-sh/setup-uv@v4stepThe
astral-sh/setup-uvaction respects the Python version set byactions/setup-python, so the current workflow will function. However, to ensure uv creates environments for each matrix Python version, thepython-versioninput (orUV_PYTHONenvironment variable) should be explicitly set.Update the setup-uv step:
- name: Install UV uses: astral-sh/setup-uv@v4 with: version: "${{ needs.prepare-environment.outputs.UV_VERSION }}" python-version: ${{ matrix.python-version }}This makes the Python version selection explicit and ensures robust behavior across the full matrix.
🧹 Nitpick comments (6)
.gitignore (1)
11-12: Consider removing the redundant.venvpattern.Both
.venv/and.venvpatterns are present; however,.venv/alone is sufficient to ignore the virtual environment directory. The.venvpattern is technically redundant since git only tracks files within directories, not the directory itself.Consider applying this diff to remove the redundancy:
-.venv/ -.venv +.venv/Alternatively, if you prefer maximum clarity about intent, keeping both is harmless and acceptable.
AGENTS.md (1)
106-106: Optional: Consider hyphenating "on demand".Static analysis suggests "on-demand" when used as a compound adjective. This is a stylistic choice and can be deferred.
infrahub_sdk/diff.py (1)
41-52: Diff tree metadata type and Query builder align with client usage
DiffTreeDataaccurately captures the metadata and node list shape used by the newget_diff_treeclient methods, andget_diff_tree_query()builds aQuerythat:
- Filters
DiffTreebybranch,name,from_time,to_time.- Selects the expected metadata fields (
num_*,{from,to}_time,base_branch,diff_branch,name) plus the full node structure thatdiff_tree_node_to_node_diffconsumes.- Declares variables with appropriate Python types (
str,str | None,datetime | None), matching the new DateTime variable support.Only minor nit is that the GraphQL operation name
"GetDiffTree"is now used by both the older string-based query and this newQueryobject, which is allowed but can be slightly confusing when debugging; not a blocker.Also applies to: 146-206
tests/unit/ctl/test_branch_report.py (1)
12-38: Good coverage for timestamp formatting, Git diff helper, and branch report CLIThese tests thoroughly exercise:
format_timestampacross valid ISO, timezone-aware, invalid, andNoneinputs.check_git_files_changedfor files present, absent, empty responses, and HTTP errors.- The
branch reportCLI for both no proposed changes and complex proposed-change cases includingis_draft, approvals/rejections, and creator info.One small consideration: assertions like
"Is draft No"rely on exact Rich table spacing and may become fragile if column widths change; if that starts causing churn, you might relax them to focus on content rather than alignment.Also applies to: 40-126, 128-426
infrahub_sdk/ctl/branch.py (2)
26-33: Broadenformat_timestamptyping to match actual usage
format_timestampcurrently annotatestimestamp: str -> str, but it gracefully handlesNone(and tests rely onNonereturningNone). To better reflect reality and keep type-checkers happy, consider:def format_timestamp(timestamp: str | None) -> str | None: ...The implementation itself looks correct for the formats you’re handling.
189-294: Branch report flow is solid; a couple of small cleanupsThe
reportcommand nicely ties together:
- Branch metadata (creation time, status, sync_with_git, schema changes).
- Optional diff creation from
branched_fromto now (update_diffflag).- Retrieval of the diff tree and Git changes, plus a summary table of diff counts.
- Proposed changes listing with creator, draft status, approvals, and rejections.
Two small suggestions:
Git files changed handling
You initialize
git_files_changed = Noneand then immediately overwrite it with the result ofcheck_git_files_changed, so the"N/A"branch is never taken. If the intent was to show"N/A"for branches not synced with Git, you could make that explicit:git_files_changed = None if branch.sync_with_git: git_files_changed = await check_git_files_changed(client, branch=branch_name)Timestamp parsing robustness
Parsing
branch.branched_fromviadatetime.fromisoformat(branch.branched_from.replace("Z", "+00:00"))is fine for current formats, but if that field ever becomes a nativedatetime, you’ll hit anAttributeError. A small isinstance guard would make this safer, though not strictly required right now.Overall, the command behavior is clear and matches the new tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
poetry.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
.github/workflows/ci.yml(6 hunks).github/workflows/define-versions.yml(1 hunks).github/workflows/publish-pypi.yml(1 hunks).github/workflows/release.yml(2 hunks).gitignore(1 hunks).vale/styles/spelling-exceptions.txt(3 hunks)AGENTS.md(1 hunks)CLAUDE.md(4 hunks)README.md(1 hunks)docs/AGENTS.md(1 hunks)docs/docs/python-sdk/guides/client.mdx(1 hunks)infrahub_sdk/client.py(3 hunks)infrahub_sdk/ctl/AGENTS.md(1 hunks)infrahub_sdk/ctl/branch.py(3 hunks)infrahub_sdk/ctl/cli.py(1 hunks)infrahub_sdk/diff.py(3 hunks)infrahub_sdk/graphql/constants.py(1 hunks)infrahub_sdk/graphql/renderers.py(2 hunks)infrahub_sdk/node/attribute.py(1 hunks)infrahub_sdk/node/related_node.py(2 hunks)infrahub_sdk/pytest_plugin/AGENTS.md(1 hunks)pyproject.toml(3 hunks)tasks.py(1 hunks)tests/AGENTS.md(1 hunks)tests/unit/ctl/test_branch_report.py(1 hunks)tests/unit/sdk/test_diff_summary.py(2 hunks)
🧰 Additional context used
🪛 LanguageTool
AGENTS.md
[uncategorized] ~106-~106: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...Nodes load attributes and relationships on demand - Batch Operations: Support for bul...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ 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). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (29)
.vale/styles/spelling-exceptions.txt (1)
23-23: LGTM!The spelling exception additions are appropriate and correctly ordered alphabetically. All four terms are legitimate:
callouts: Common documentation termDiataxis: Documentation methodology frameworkfrontmatter: Standard documentation/markdown termTyper: CLI framework used in this codebaseAlso applies to: 27-27, 47-47, 120-120
docs/docs/python-sdk/guides/client.mdx (1)
20-20: LGTM!The installation documentation correctly reflects the migration from Poetry to UV, providing users with the updated command while maintaining the pip installation option.
infrahub_sdk/ctl/cli.py (1)
7-8: LGTM!The error message has been appropriately updated to reflect the UV-based workflow while maintaining the pip installation option for standard users.
README.md (1)
59-73: LGTM!The new UV development setup section is well-documented and provides clear examples for different use cases. The commands are correct and will help developers understand how to set up their development environment with UV.
tasks.py (1)
42-44: LGTM!The documentation generation commands have been correctly updated to use UV instead of Poetry, maintaining consistency with the broader tooling migration.
Also applies to: 52-52
infrahub_sdk/node/attribute.py (1)
108-108: LGTM!Adding
updated_atto the query data structure is appropriate and aligns with the existingupdated_atattribute defined at line 53. This ensures the field is consistently available in generated queries.infrahub_sdk/graphql/renderers.py (1)
4-4: Verification complete: VARIABLE_TYPE_MAPPING supports datetime.The import of
datetimeininfrahub_sdk/graphql/renderers.py(line 4) is properly supported by theVARIABLE_TYPE_MAPPINGininfrahub_sdk/graphql/constants.py, which includes both(datetime, "DateTime!")and(datetime | None, "DateTime")mappings. The changes are correct and well-aligned.AGENTS.md (1)
1-245: Comprehensive guidance for AI coding assistants.This documentation provides excellent coverage of the repository structure, development setup, architecture, conventions, and domain-specific guidance. The structure is clear and will be valuable for AI coding assistants working with the codebase.
infrahub_sdk/pytest_plugin/AGENTS.md (1)
1-324: Excellent pytest plugin development documentation.This comprehensive guide covers the pytest plugin architecture, YAML test file format, test item classes, configuration, and common patterns. The structure is clear with practical examples that will be valuable for developers working on the custom pytest plugin.
tests/AGENTS.md (1)
1-366: Comprehensive testing guidance.This documentation provides excellent coverage of the testing framework, directory organization, running tests, pytest configuration, fixtures, and common patterns. The examples are clear and will help developers write consistent, high-quality tests.
CLAUDE.md (3)
3-5: Appropriate deprecation notice.The deprecation notice correctly directs users to the new AGENTS.md standard while maintaining backward compatibility.
15-54: UV migration commands look correct.All development commands have been properly migrated from Poetry to UV syntax, including dependency installation, formatting, linting, testing, and documentation generation.
181-181: Python version range documented.Line 181 documents CI/CD testing on Python 3.10-3.13. This should align with the Python version support documented in AGENTS.md line 19 (currently states 3.9-3.13). Please ensure consistency across all documentation.
docs/AGENTS.md (1)
1-357: Excellent documentation development guidance.This comprehensive guide covers the Docusaurus-based documentation system, Diataxis framework, directory structure, development workflow, writing guidelines, MDX components, and code quality practices. The structure and examples will be valuable for developers contributing to documentation.
infrahub_sdk/ctl/AGENTS.md (1)
1-314: Comprehensive CLI development guidance.This documentation provides excellent coverage of the
infrahubctlCLI architecture, command structure, AsyncTyper patterns, code conventions, error handling, testing, and common patterns. The practical examples and file organization details will be valuable for developers working on CLI commands..github/workflows/release.yml (3)
33-39: Proper UV setup and dependency installation.The workflow correctly migrates from Poetry to UV, using
astral-sh/setup-uv@v4anduv sync --all-groups --all-extrasfor dependency installation.
44-48: Correct version extraction using UV and tomllib.The version extraction properly uses
uv run pythonwithtomllibto read frompyproject.toml, and correctly determines prerelease/devrelease status using thepackaging.version.Versionclass. This is the appropriate approach for PEP 621-compliant projects.
58-64: Improved error messaging for tag verification.The tag version check now provides clear "Expected vs Got" output when there's a mismatch, improving debugging experience.
.github/workflows/publish-pypi.yml (1)
44-64: UV setup and dependency caching look consistentUsing
astral-sh/setup-uv@v4, caching.venvkeyed onuv.lock, and runninguv sync --all-groups --all-extrasis a good migration away from Poetry here. No issues spotted with these steps.pyproject.toml (2)
1-104: PEP 621 + hatchling migration looks coherentThe move to
[project]metadata, optional-dependencies forctl/all, dependency-groups fortests/lint/types/dev, andhatchlingas the build backend all look consistent with the new uv-based workflow. The script and pytest plugin entry points are wired correctly to existing modules.
347-352: Build backend configuration matches package layoutSpecifying
requires = ["hatchling"],build-backend = "hatchling.build", and limiting wheel packages to["infrahub_sdk"]aligns with the rest of this repo’s structure and the CI workflows usinguv build..github/workflows/ci.yml (2)
161-187: Generated-docs validation job’s uv usage is consistentFor
validate-generated-documentation, setting up Python 3.12, installing uv, runninguv sync --all-groups --all-extras, and thenuv run invoke docs-validatefrom the repo root matches the new pyproject/uv setup and should be fine.
270-302: Integration tests updated correctly to use uvSwitching the integration job to:
- install uv,
uv sync --all-groups --all-extras,uv run pytest --cov infrahub_sdk tests/integration/,uv run codecov --flags integration-tests,fits the new dependency management model and reuses the same tooling as unit tests.
infrahub_sdk/node/related_node.py (1)
67-80: Consistentupdated_atsupport for related nodesReading
updated_atfrom either the top-level data orpropertiesand always including"updated_at": Nonein_generate_query_datagives related nodes a stableupdated_atattribute (needed by consumers like the branch report). The change does alter the GraphQL selection to always include anupdated_atfield inproperties, but that’s a reasonable trade-off for consistency.Also applies to: 165-176
tests/unit/sdk/test_diff_summary.py (1)
1-8: Comprehensive tests for diff tree query and client behaviorThe additions:
- Validate the rendered structure of
get_diff_tree_query()(operation name, variables, core fields).- Exercise
get_diff_treeon both async and sync clients with full metadata, no diff, and filtered (name + time range) scenarios.- Confirm time-range validation by asserting
from_time > to_timeraisesValueError.This gives solid coverage around the new diff tree functionality and should catch regressions in both query construction and client-side handling.
Also applies to: 166-405
infrahub_sdk/ctl/branch.py (1)
35-59:check_git_files_changedlogic is soundBuilding the diff/files URL from
client.address, usingclient._getwith the client’s default timeout, and decoding JSON viadecode_jsonis consistent with the rest of the SDK. The iteration overdata[branch].values()to find any repo with a non-emptyfileslist correctly returnsTruewhen changes exist andFalseotherwise.No functional issues here.
infrahub_sdk/client.py (3)
38-38: LGTM! Import statement adds necessary dependencies.The import adds the required types and functions for the new
get_diff_treemethod. Also includesget_diff_summary_querywhich was previously used but not explicitly imported.
2582-2638: Verify query type consistency (sync version).The sync implementation has the same potential inconsistency as the async version. Line 2608 calls
query.render(), while the similarget_diff_summarymethod (line 2563) passes the query directly. Please ensure bothget_diff_tree_query()andget_diff_summary_query()are being used consistently according to their return types.
1286-1342: No issues found—the query builder pattern is correct as implemented.The apparent inconsistency between calling
.render()onget_diff_tree_query()(line 1312) and passingget_diff_summary_query()directly (line 1256) is intentional and correct. Verification confirms:
get_diff_tree_query()returns aQueryobject with a.render()method that produces a GraphQL stringget_diff_summary_query()returns a raw GraphQL string directlyexecute_graphql()expects aquery: strparameter in both cases- Each usage correctly handles its respective query builder's return type
The two query builders are designed differently—one uses a programmatic Query object pattern, the other returns a raw string—and the implementation correctly applies each pattern.
Likely an incorrect or invalid review comment.
| UV_VERSION: | ||
| value: "0.9.8" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the UV version is current and appropriate.
The workflow output has been correctly renamed from POETRY_VERSION to UV_VERSION with version "0.9.8". Please confirm that this version is appropriate and that all consuming workflows have been updated to reference UV_VERSION instead of POETRY_VERSION.
🌐 Web query:
What is the latest stable version of UV (uv package manager)?
💡 Result:
The latest stable release is uv 0.9.9 — released November 12, 2025. [1]
Sources:
[1] astral-sh/uv — Releases (GitHub).
🏁 Script executed:
#!/bin/bash
# Search for UV_VERSION and POETRY_VERSION references in workflows
echo "=== Searching for UV_VERSION references in workflows ==="
rg -n "UV_VERSION" .github/workflows/ --type yaml
echo ""
echo "=== Searching for POETRY_VERSION references in workflows ==="
rg -n "POETRY_VERSION" .github/workflows/ --type yaml
echo ""
echo "=== Searching for outputs from define-versions workflow ==="
rg -n "define-versions" .github/workflows/ --type yaml -A 3 -B 3Length of output: 2246
Update UV_VERSION to current stable release (0.9.9).
The latest stable version of UV is 0.9.9, released November 12, 2025. The current version "0.9.8" in the workflow is outdated. All consuming workflows (release.yml, publish-pypi.yml, ci.yml) correctly reference UV_VERSION, and the POETRY_VERSION reference is commented out, so the migration is complete. Update line 8 in .github/workflows/define-versions.yml to set the version to "0.9.9".
🤖 Prompt for AI Agents
In .github/workflows/define-versions.yml around lines 7 to 8, the UV_VERSION
value is set to the outdated "0.9.8"; update that line to set UV_VERSION to the
current stable release "0.9.9" so dependent workflows pick up the correct
version.
wvandeun
force-pushed
the
wvd-20251114-infp388-branch-cleanup-mechanism
branch
from
fd46ca8 to
fcbb594
Compare
github-actions
bot
added
the
type/documentation
wvandeun
force-pushed
the
wvd-20251114-infp388-branch-cleanup-mechanism
branch
from
fcbb594 to
86bc4b9
Compare
wvandeun
changed the title
infrahubctl bracnh report commandinfrahubctl branch report command
github-actions
bot
removed
the
type/documentation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
github-actions
bot
added
the
type/documentation
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## infrahub-develop #637 +/- ##
====================================================
+ Coverage 75.60% 75.79% +0.19%
====================================================
Files 113 113
Lines 9522 9657 +135
Branches 1452 1472 +20
====================================================
+ Hits 7199 7320 +121
- Misses 1832 1841 +9
- Partials 491 496 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Replace runtime | operator with Union from typing to support Python 3.9-3.13: - graphql/constants.py: VARIABLE_TYPE_MAPPING tuples - diff.py: GraphQL query variables dict The | operator for types only works at runtime in Python 3.10+.
wvandeun
force-pushed
the
wvd-20251114-infp388-branch-cleanup-mechanism
branch
from
576691a to
78c71b3
Compare
wvandeun
force-pushed
the
wvd-20251114-infp388-branch-cleanup-mechanism
branch
from
9fe6467 to
634d2d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
infrahub_sdk/object_store.py (1)
35-42: Set-based status checks are fine, but non-auth HTTP errors are silently ignoredThe change from a list to a set for
{401, 403}is fine and slightly more efficient for membership tests.More importantly, in all four
except httpx.HTTPStatusErrorblocks, non-401/403 HTTP errors are currently swallowed: for any other status code, the code falls through and returnsresp.text/resp.json()as if the request had succeeded. That makes it hard to detect 4xx/5xx failures from callers.Consider explicitly re-raising the exception when it’s not an auth error, for both async and sync methods, for example:
except httpx.HTTPStatusError as exc: - if exc.response.status_code in {401, 403}: - response = exc.response.json() - errors = response.get("errors") - messages = [error.get("message") for error in errors] - raise AuthenticationError(" | ".join(messages)) from exc + if exc.response.status_code in {401, 403}: + response = exc.response.json() + errors = response.get("errors") + messages = [error.get("message") for error in errors] + raise AuthenticationError(" | ".join(messages)) from exc + raiseThis preserves the specialized AuthenticationError path while surfacing other HTTP failures to callers.
Also applies to: 56-62, 83-90, 104-111
infrahub_sdk/ctl/branch.py (1)
116-116: Bug: Incorrect variable used forhas_schema_changes.Line 116 references
default_branch.has_schema_changesinstead ofbranch.has_schema_changes, causing all non-default branches to display the default branch's schema change status instead of their own.- "[green]True" if default_branch.has_schema_changes else "[#FF7F50]False", + "[green]True" if branch.has_schema_changes else "[#FF7F50]False",
🧹 Nitpick comments (11)
infrahub_sdk/protocols_generator/generator.py (1)
53-60: Set-based exclusion forbase_protocolsis correct and consistentUsing a set literal for the exclusion list keeps behavior the same and improves membership-check performance; the comprehension still produces a
list[str]as before, so downstream uses (e.g., in_sort_and_filter_models) remain valid.If you want to go one step further, you could move the exclusion set into a named module-level constant (e.g.,
BASE_PROTOCOL_EXCLUDES: set[str] = {...}) to make it reusable and slightly clearer, but that’s purely optional.tests/unit/sdk/checks/test_checks.py (1)
11-11: Switch totmp_pathfixture and typed root_directory usage looks goodUsing
tmp_path: Pathand passingroot_directory=str(tmp_path)removes the hard-coded/tmp, improves test isolation, and keeps the check’sroot_directoryexpectation as a string while still being mypy‑friendly via the type hint. This aligns well with the strict typing guideline for tests.As per coding guidelines, this also supports strict mypy by making the fixture type explicit.
Also applies to: 34-36
infrahub_sdk/ctl/importer.py (1)
17-19: Confirm intentional change in default directory semantics (symlink resolution).Switching
local_directorytoPath.cwd()means the default directory will now reflect the working directory as-is, without resolving symlinks, whereas the previousPath().resolve()would have returned the canonical real path. This only affects behavior when the current working directory is a symlink or when realpath vs. symlinked path matters for downstream consumers.If the goal is strictly “use whatever cwd the process sees”, this is fine. If you still want canonical paths while avoiding doc-generation issues,
Path.cwd().resolve()might be a middle ground.tests/fixtures/integration/test_infrahubctl/tags_transform/tags_transform.py (1)
8-13: Return type annotation matches implementation; consider also typingdata.The added
-> dict[str, str]accurately reflects the returned literal. For better type safety and stricter checking, you may also want to annotatedata(e.g., as a suitably structuredMapping/dict), so the function is fully typed end‑to‑end. This is optional but would align well with a strict typing setup. As per coding guidelines, ensuring strong typing here will help mypy catch mismatches earlier.tests/unit/ctl/test_repository_app.py (1)
19-23: mock_client fixture simplification looks fineReturning
mock.create_autospec(InfrahubClient)keeps the existing behavior and gives you spec-checked attributes. If you ever need this to behave more like an instantiated client (e.g., bound methods withselfin signatures), you could tighten it slightly withinstance=True, but that’s optional given current usage.infrahub_sdk/config.py (2)
170-177: Logger property type-ignore is acceptable; consider Optional/cast to tighten types.The narrower
# type: ignore[return-value]keeps the workaround localized, which is fine. If you ever revisit typing here, you might consider either makinglog/loggerexplicitlyInfrahubLoggers | Noneor usingcast(InfrahubLoggers, self.log)so mypy understands the intent without an ignore.
187-201: clone() logic remains correct; you can simplify membership checks.Iterating
Config.model_fieldsdirectly is fine and behaves like iterating its keys. You can dropcovered_keys = list(config.keys())and test membership on the dict itself (if field not in config:) to avoid the extra list allocation.tests/unit/ctl/test_transform_app.py (1)
3-40: Fixture Generator annotation is fine; you can tighten it if desired.Using
collections.abc.Generatorfortags_transform_dirmatches the yield-based fixture pattern; if you want stricter typing, considerGenerator[str, None, None]orIterator[str]to make the send/return types explicit.tests/unit/sdk/conftest.py (1)
65-145: Type-aware replace_*_annotation helpers look correct; small robustness improvements are optional.The new Callable/Mapping-based type hints on
replace_async_return_annotation,replace_async_parameter_annotations,replace_sync_return_annotation, andreplace_sync_parameter_annotationsalign with how these fixtures are used to compare async vs sync signatures. If you ever need to handle unannotated parameters, you might want to guard againstinspect._emptyinparameter.annotation, but for the current strictly-annotated targets this is fine.tests/integration/test_infrahub_client.py (1)
36-41: Consider adding type parameters toAsyncGenerator.The return type could be more specific:
AsyncGenerator[None, None]to indicate the fixture yields None and doesn't accept sent values.@pytest.fixture - async def set_pagination_size3(self, client: InfrahubClient) -> AsyncGenerator: + async def set_pagination_size3(self, client: InfrahubClient) -> AsyncGenerator[None, None]: original_pagination_size = client.pagination_size client.pagination_size = 3 yield client.pagination_size = original_pagination_sizeinfrahub_sdk/ctl/branch.py (1)
226-227: Remove redundant assignment.
git_files_changedis assignedNoneon line 226 and immediately reassigned on line 227.- git_files_changed = None - git_files_changed = await check_git_files_changed(client, branch=branch_name) + git_files_changed = await check_git_files_changed(client, branch=branch_name)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (62)
.github/workflows/ci.yml(5 hunks)CLAUDE.md(1 hunks)changelog/+f97cdf92.removed.md(1 hunks)changelog/389.changed.md(1 hunks)docs/docs/infrahubctl/infrahubctl-branch.mdx(2 hunks)infrahub_sdk/async_typer.py(1 hunks)infrahub_sdk/batch.py(1 hunks)infrahub_sdk/client.py(11 hunks)infrahub_sdk/config.py(2 hunks)infrahub_sdk/ctl/branch.py(4 hunks)infrahub_sdk/ctl/cli_commands.py(1 hunks)infrahub_sdk/ctl/graphql.py(2 hunks)infrahub_sdk/ctl/importer.py(1 hunks)infrahub_sdk/ctl/utils.py(1 hunks)infrahub_sdk/diff.py(3 hunks)infrahub_sdk/graphql/constants.py(1 hunks)infrahub_sdk/graphql/renderers.py(2 hunks)infrahub_sdk/node/attribute.py(4 hunks)infrahub_sdk/node/constants.py(1 hunks)infrahub_sdk/node/node.py(8 hunks)infrahub_sdk/node/related_node.py(1 hunks)infrahub_sdk/node/relationship.py(1 hunks)infrahub_sdk/object_store.py(4 hunks)infrahub_sdk/operation.py(1 hunks)infrahub_sdk/protocols_generator/generator.py(1 hunks)infrahub_sdk/pytest_plugin/items/jinja2_transform.py(1 hunks)infrahub_sdk/pytest_plugin/models.py(1 hunks)infrahub_sdk/pytest_plugin/plugin.py(1 hunks)infrahub_sdk/query_groups.py(2 hunks)infrahub_sdk/schema/__init__.py(5 hunks)infrahub_sdk/schema/main.py(1 hunks)infrahub_sdk/schema/repository.py(2 hunks)infrahub_sdk/spec/object.py(2 hunks)infrahub_sdk/spec/range_expansion.py(1 hunks)infrahub_sdk/template/__init__.py(1 hunks)infrahub_sdk/transfer/importer/json.py(3 hunks)infrahub_sdk/types.py(2 hunks)infrahub_sdk/utils.py(2 hunks)pyproject.toml(4 hunks)tests/__init__.py(1 hunks)tests/conftest.py(3 hunks)tests/fixtures/integration/test_infrahubctl/tags_transform/tags_transform.py(1 hunks)tests/helpers/fixtures.py(1 hunks)tests/integration/conftest.py(1 hunks)tests/integration/test_infrahub_client.py(4 hunks)tests/unit/__init__.py(1 hunks)tests/unit/ctl/conftest.py(2 hunks)tests/unit/ctl/test_branch_report.py(1 hunks)tests/unit/ctl/test_render_app.py(1 hunks)tests/unit/ctl/test_repository_app.py(1 hunks)tests/unit/ctl/test_transform_app.py(2 hunks)tests/unit/sdk/checks/test_checks.py(3 hunks)tests/unit/sdk/conftest.py(48 hunks)tests/unit/sdk/graphql/conftest.py(6 hunks)tests/unit/sdk/graphql/test_plugin.py(2 hunks)tests/unit/sdk/test_batch.py(1 hunks)tests/unit/sdk/test_diff_summary.py(2 hunks)tests/unit/sdk/test_node.py(42 hunks)tests/unit/sdk/test_repository.py(2 hunks)tests/unit/sdk/test_schema.py(1 hunks)tests/unit/sdk/test_template.py(1 hunks)tests/unit/sdk/test_yaml.py(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- changelog/389.changed.md
- tests/unit/sdk/test_template.py
- docs/docs/infrahubctl/infrahubctl-branch.mdx
- tests/unit/init.py
- changelog/+f97cdf92.removed.md
- tests/unit/sdk/test_repository.py
- tests/integration/conftest.py
- infrahub_sdk/batch.py
🚧 Files skipped from review as they are similar to previous changes (5)
- CLAUDE.md
- tests/unit/sdk/test_diff_summary.py
- infrahub_sdk/graphql/renderers.py
- tests/unit/ctl/test_branch_report.py
- infrahub_sdk/graphql/constants.py
🧰 Additional context used
📓 Path-based instructions (8)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All async operations must have corresponding sync implementations following the dual implementation pattern
Use mypy with strict configuration for type checking in Python files
Files:
tests/unit/sdk/graphql/test_plugin.pyinfrahub_sdk/diff.pytests/unit/sdk/test_yaml.pytests/helpers/fixtures.pyinfrahub_sdk/async_typer.pyinfrahub_sdk/utils.pyinfrahub_sdk/schema/main.pytests/conftest.pyinfrahub_sdk/ctl/importer.pytests/unit/sdk/test_batch.pyinfrahub_sdk/protocols_generator/generator.pytests/unit/sdk/test_schema.pyinfrahub_sdk/object_store.pyinfrahub_sdk/transfer/importer/json.pyinfrahub_sdk/pytest_plugin/models.pyinfrahub_sdk/schema/repository.pyinfrahub_sdk/query_groups.pyinfrahub_sdk/ctl/branch.pytests/unit/ctl/test_repository_app.pytests/fixtures/integration/test_infrahubctl/tags_transform/tags_transform.pytests/__init__.pytests/unit/sdk/checks/test_checks.pyinfrahub_sdk/node/constants.pyinfrahub_sdk/pytest_plugin/items/jinja2_transform.pyinfrahub_sdk/spec/object.pyinfrahub_sdk/node/related_node.pytests/unit/ctl/conftest.pytests/unit/sdk/test_node.pyinfrahub_sdk/operation.pytests/unit/ctl/test_render_app.pytests/unit/sdk/graphql/conftest.pyinfrahub_sdk/client.pyinfrahub_sdk/ctl/graphql.pyinfrahub_sdk/ctl/utils.pyinfrahub_sdk/types.pyinfrahub_sdk/node/attribute.pyinfrahub_sdk/schema/__init__.pytests/unit/ctl/test_transform_app.pyinfrahub_sdk/node/node.pyinfrahub_sdk/pytest_plugin/plugin.pyinfrahub_sdk/template/__init__.pytests/unit/sdk/conftest.pyinfrahub_sdk/node/relationship.pyinfrahub_sdk/ctl/cli_commands.pyinfrahub_sdk/config.pyinfrahub_sdk/spec/range_expansion.pytests/integration/test_infrahub_client.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place unit tests in
tests/unit/directory for testing individual components in isolation
Files:
tests/unit/sdk/graphql/test_plugin.pytests/unit/sdk/test_yaml.pytests/unit/sdk/test_batch.pytests/unit/sdk/test_schema.pytests/unit/ctl/test_repository_app.pytests/unit/sdk/checks/test_checks.pytests/unit/ctl/conftest.pytests/unit/sdk/test_node.pytests/unit/ctl/test_render_app.pytests/unit/sdk/graphql/conftest.pytests/unit/ctl/test_transform_app.pytests/unit/sdk/conftest.py
**/ctl/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Typer framework for building CLI commands in the infrahubctl CLI
Files:
infrahub_sdk/ctl/importer.pyinfrahub_sdk/ctl/branch.pytests/unit/ctl/test_repository_app.pytests/unit/ctl/conftest.pytests/unit/ctl/test_render_app.pyinfrahub_sdk/ctl/graphql.pyinfrahub_sdk/ctl/utils.pytests/unit/ctl/test_transform_app.pyinfrahub_sdk/ctl/cli_commands.py
**/node/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/node/*.py: Implement lazy loading for node attributes and relationships to load on demand
Support batch operations for bulk create/update/delete operations on nodes
Include built-in data validation with GraphQL query generation for nodes
Files:
infrahub_sdk/node/constants.pyinfrahub_sdk/node/related_node.pyinfrahub_sdk/node/attribute.pyinfrahub_sdk/node/node.pyinfrahub_sdk/node/relationship.py
**/client.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/client.py: UseInfrahubClient(async) andInfrahubClientSync(sync) with identical interfaces following the Dual Client Pattern
Use HTTPX-based transport with proxy support (single proxy or HTTP/HTTPS mounts) and API token/JWT authentication with automatic refresh
Files:
infrahub_sdk/client.py
**/node/relationship.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement automatic handling of node relationships with add/remove/replace operations
Files:
infrahub_sdk/node/relationship.py
**/config.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/config.py: Use Pydantic-basedConfigclass with environment variable support for configuration management
Use environment variables prefixed withINFRAHUB_for configuration
Ensure mutual exclusivity validation between proxy configuration methods (INFRAHUB_PROXYvsINFRAHUB_PROXY_MOUNTS_HTTP/INFRAHUB_PROXY_MOUNTS_HTTPS)
Files:
infrahub_sdk/config.py
tests/integration/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place integration tests in
tests/integration/directory for testing against real Infrahub instances
Files:
tests/integration/test_infrahub_client.py
🧠 Learnings (9)
📚 Learning: 2025-11-25T07:18:58.136Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T07:18:58.136Z
Learning: Applies to **/node/*.py : Include built-in data validation with GraphQL query generation for nodes
Applied to files:
infrahub_sdk/diff.pytests/unit/sdk/test_node.py
📚 Learning: 2025-11-25T07:18:58.135Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T07:18:58.135Z
Learning: Applies to **/node/relationship.py : Implement automatic handling of node relationships with add/remove/replace operations
Applied to files:
infrahub_sdk/transfer/importer/json.pyinfrahub_sdk/node/related_node.pyinfrahub_sdk/operation.pyinfrahub_sdk/node/node.pyinfrahub_sdk/node/relationship.py
📚 Learning: 2025-11-25T07:18:58.135Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T07:18:58.135Z
Learning: Applies to **/client.py : Use `InfrahubClient` (async) and `InfrahubClientSync` (sync) with identical interfaces following the Dual Client Pattern
Applied to files:
infrahub_sdk/schema/repository.pyinfrahub_sdk/ctl/branch.pytests/unit/ctl/test_repository_app.pytests/unit/sdk/checks/test_checks.pyinfrahub_sdk/client.pyinfrahub_sdk/schema/__init__.pytests/unit/sdk/conftest.py
📚 Learning: 2025-11-25T07:18:58.136Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T07:18:58.136Z
Learning: Applies to **/ctl/**/*.py : Use Typer framework for building CLI commands in the infrahubctl CLI
Applied to files:
infrahub_sdk/ctl/branch.pyinfrahub_sdk/ctl/cli_commands.py
📚 Learning: 2025-11-25T07:18:58.136Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T07:18:58.136Z
Learning: Run linting with `poetry run invoke lint` (ruff + mypy + yamllint + markdownlint) to ensure code quality
Applied to files:
pyproject.toml.github/workflows/ci.yml
📚 Learning: 2025-11-25T07:18:58.136Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T07:18:58.136Z
Learning: Use Ruff (0.11.0) for comprehensive linting and formatting
Applied to files:
pyproject.toml.github/workflows/ci.yml
📚 Learning: 2025-11-25T07:18:58.136Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T07:18:58.136Z
Learning: Applies to tests/integration/**/*.py : Place integration tests in `tests/integration/` directory for testing against real Infrahub instances
Applied to files:
tests/unit/sdk/checks/test_checks.py
📚 Learning: 2025-11-25T07:18:58.136Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T07:18:58.136Z
Learning: Run unit tests with `poetry run pytest --cov infrahub_sdk tests/unit/` to ensure code coverage
Applied to files:
tests/unit/sdk/checks/test_checks.py
📚 Learning: 2025-11-25T07:18:58.135Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T07:18:58.135Z
Learning: Applies to **/node/*.py : Implement lazy loading for node attributes and relationships to load on demand
Applied to files:
infrahub_sdk/node/node.py
🧬 Code graph analysis (11)
tests/unit/sdk/test_yaml.py (1)
infrahub_sdk/yaml.py (2)
YamlFile(42-140)load_content(43-58)
infrahub_sdk/transfer/importer/json.py (3)
infrahub_sdk/node/related_node.py (1)
kind(123-126)infrahub_sdk/schema/main.py (1)
kind(279-280)infrahub_sdk/yaml.py (1)
kind(157-158)
infrahub_sdk/schema/repository.py (1)
infrahub_sdk/node/node.py (2)
InfrahubNode(458-1108)InfrahubNodeSync(1111-1755)
infrahub_sdk/query_groups.py (1)
infrahub_sdk/node/relationship.py (1)
peer_ids(44-45)
tests/unit/sdk/test_node.py (2)
tests/unit/sdk/conftest.py (3)
BothClients(25-28)client(32-33)clients(42-46)infrahub_sdk/node/node.py (6)
InfrahubNode(458-1108)InfrahubNodeSync(1111-1755)from_graphql(486-500)from_graphql(1139-1153)_process_relationships(892-945)_process_relationships(1539-1592)
infrahub_sdk/operation.py (1)
infrahub_sdk/node/node.py (4)
from_graphql(486-500)from_graphql(1139-1153)_process_relationships(892-945)_process_relationships(1539-1592)
infrahub_sdk/client.py (1)
infrahub_sdk/diff.py (4)
DiffTreeData(39-49)NodeDiff(11-17)diff_tree_node_to_node_diff(91-141)get_diff_tree_query(144-204)
infrahub_sdk/node/attribute.py (3)
infrahub_sdk/yaml.py (1)
data(147-150)infrahub_sdk/node/related_node.py (1)
id(83-86)infrahub_sdk/protocols_base.py (1)
id(15-15)
infrahub_sdk/schema/__init__.py (1)
infrahub_sdk/schema/main.py (6)
NodeSchema(307-309)GenericSchema(283-285)NodeSchemaAPI(312-314)GenericSchemaAPI(288-292)ProfileSchemaAPI(317-318)TemplateSchemaAPI(321-322)
tests/unit/sdk/conftest.py (2)
infrahub_sdk/schema/main.py (4)
NodeSchema(307-309)convert_api(284-285)convert_api(308-309)NodeSchemaAPI(312-314)infrahub_sdk/yaml.py (1)
data(147-150)
tests/integration/test_infrahub_client.py (2)
tests/unit/sdk/conftest.py (1)
client(32-33)infrahub_sdk/client.py (1)
InfrahubClient(318-1746)
⏰ 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). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (90)
tests/unit/sdk/test_batch.py (1)
68-76: Idiomatic use ofanextfor async iteration looks goodSwitching from calling
__anext__()directly toawait anext(result_iter)is clearer, more idiomatic, and matches the Python 3.10+ target in this repo. No further changes needed here.tests/helpers/fixtures.py (1)
13-14: LGTM! Clean refactoring.The direct return eliminates an unnecessary intermediate variable, making the code more concise while maintaining the same functionality. The explicit UTF-8 encoding specification is a best practice.
tests/__init__.py (1)
6-7: LGTM! More specific type ignore improves type safety.Refining the type ignore comments from generic to
[attr-defined]is a best practice that makes the suppression more targeted, allowing mypy to catch other potential issues on these lines.infrahub_sdk/spec/object.py (2)
77-77: Good use of set literal for membership check.Changing from a list to a set literal for membership testing is more efficient and idiomatic Python.
233-233: Good simplification of dict membership check.Removing the explicit
.keys()call is more idiomatic, asinon a dict checks keys by default.tests/unit/sdk/checks/test_checks.py (1)
1-1: Path import is appropriate and scoped correctlyThe new
Pathimport is used for thetmp_pathtype annotation and keeps dependencies minimal; no issues here.tests/unit/ctl/conftest.py (1)
1-1: LGTM! Type annotation and code simplification improvements.The addition of the
Anyimport and the updated type annotationdict[str, Any]improve type safety using modern Python syntax. Removing the intermediate variable simplifies the fixture without changing behavior.Also applies to: 68-77
infrahub_sdk/async_typer.py (1)
5-8: Callable import migration is correct and non-breakingSwitching to
collections.abc.Callable(and keepingAnyfromtyping) is idiomatic for Python 3.10+ and doesn’t alterAsyncTyperbehavior. The async/sync dispatch inmaybe_run_async,callback, andcommandremains the same.Also applies to: 12-32
infrahub_sdk/ctl/utils.py (1)
6-10: Typing refactor to collections.abc is soundUsing
CallableandCoroutinefromcollections.abcwith the existingcatch_exceptionsignature keeps behavior intact and aligns with the project-wide 3.10+ typing cleanup. No issues from a mypy-strict perspective.Also applies to: 78-86
infrahub_sdk/types.py (1)
3-6: Modernized union alias is correctChanging
InfrahubLoggerstoInfrahubLogger | Loggerand droppingUnionfrom imports is a straightforward 3.10+ typing cleanup with no runtime impact.Also applies to: 47-69
infrahub_sdk/utils.py (1)
165-176: Integer handling and explicit UTF‑8 encoding look goodUsing
{0, 1}for the integer branch instr_to_boolpreserves existing semantics, and addingencoding="utf-8"inwrite_to_filealigns with the UTF‑8 assumption already used inread_file. No behavior regressions here.Also applies to: 320-323
infrahub_sdk/pytest_plugin/items/jinja2_transform.py (1)
83-87: Explicit UTF‑8 for template parsing is appropriateReading the template with
read_text(encoding="utf-8")before passing it toparsemakes the smoke test deterministic across environments and matches other UTF‑8 I/O changes in the repo.infrahub_sdk/transfer/importer/json.py (1)
60-69: Import loop and encoding tweaks are correct and behavior‑preserving
- Using
zip(table.column("graphql_json"), table.column("kind"), strict=False)makes the non‑strict behavior explicit without changing semantics.- Iterating
self.optional_relationships_schemas_by_node_kind[node_kind]directly instead of calling.keys()is equivalent and a bit cleaner.- Reading the relationships JSON with
file.read_text(encoding="utf-8")aligns with the rest of the codebase’s encoding assumptions.Also applies to: 101-122, 146-148
infrahub_sdk/template/__init__.py (1)
3-7: Modernized Callable import looks good.Using
collections.abc.Callablewithfrom __future__ import annotationsis the right direction and keeps types consistent with the rest of the PR.infrahub_sdk/node/constants.py (1)
4-8: Extending PROPERTIES_FLAG and updating IP_TYPES is consistent with new data model.Including
"updated_at"inPROPERTIES_FLAGmatches the updated fixtures and ensures that property metadata is treated uniformly; the PEP 604 union forIP_TYPESis a pure typing cleanup.tests/unit/ctl/test_render_app.py (1)
74-85: Test signature formatting and annotation are fine.Splitting the parameters and adding
-> Nonekeeps the test clearer without changing behavior.tests/unit/sdk/conftest.py (26)
5-46: Async client fixtures and BothClients construction look consistent with the dual-client pattern.The added imports (
AsyncGenerator,Callable,Mapping,Any) and the directBothClientsconstruction inclientskeep async/sync clients aligned and make the echo_clients fixture setup straightforward; no issues spotted here.
147-223: Schema fixtures returning NodeSchemaAPI via convert_api() are consistent with the core schema models.Switching
location_schema,location_schema_with_dropdown, andstd_group_schemato buildNodeSchema(**data).convert_api()matches the pattern used elsewhere (NodeSchema.convert_api()returningNodeSchemaAPI) and keeps test schema shapes in sync with production models.
225-302: schema_with_hfid fixture and additional NodeSchemaAPI conversions look structurally sound.The nested
location/rackschema definitions, includinghuman_friendly_idlists, attributes, and relationships, match the expected NodeSchema shape, and converting each entry toNodeSchemaAPIvia a dict comprehension is both clear and correct.
305-418: location_data_no_pagination fixtures align with schema expectations.*The two no-pagination fixtures now explicitly typed as
dict[str, Any]and structure their node, attribute, and relationship fields consistently with the schema; there are no apparent mismatches in keys or nesting.
459-525: location_data01_property adds updated_at consistently across attribute and relationship properties.The new
updated_atfields under attribute property dicts and relationshippropertiesblocks line up with the extendedPROPERTIES_FLAG; this should give good coverage that updated_at is handled uniformly without changing the overall payload shape.
567-647: location_data02_property mirrors the updated_at extension for source-backed properties.The introduction of
updated_atalongsidesource/owneron both attributes and relationshippropertiesis consistent with the first location property fixture and will help validate handling when source objects are present.
651-670: rfile_userdata fixtures are now explicitly typed and remain structurally unchanged.Typing
rfile_userdata01andrfile_userdata01_propertyasdict[str, Any]clarifies intent; the structures still match the expected shape for transform Jinja2 resources.
672-684: tag_schema conversion to NodeSchemaAPI is consistent with other schema fixtures.Using
NodeSchema(**data).convert_api()here keeps tag schema fixtures aligned with the main schema models, similar to the location and group schemas.
687-765: *Tag _data_no_pagination fixtures correctly model property metadata and sources.The blue/red/green tag fixtures now carry full property dicts (including source accounts) with explicit typing; their shapes are consistent across colors and suitable for exercising property-handling logic.
768-852: *Tag _data fixtures with nested node wrappers are internally consistent.The
node-wrapped variants for blue/red/green tags mirror the no-pagination fixtures while matching the GraphQLnode/edgespattern used elsewhere; no structural issues seen.
895-916: ipaddress_schema/ipnetwork_schema/ipam_ipprefix_schema simplifications look correct.Returning
NodeSchema(**data).convert_api()for these IP-related schemas keeps test fixtures tied to the same model logic and branch handling as in production without altering the actual schema contents.
955-1052: simple_device_schema and ipam_ipprefix_data fixtures align device/IP prefix modeling.The device schema’s relationships (including pools and primary_address) and the ipam_ipPrefix data (with nested relationship properties) appear internally consistent and well-typed, giving good coverage for IPAM behaviors.
1055-1162: ipaddress_pool_schema and ipprefix_pool_schema fixtures correctly encode branch and inheritance metadata.Including
BranchSupportType.AGNOSTIC.value,inherit_from, and resource/ip_namespace relationships in these schemas matches how these entities are described in the main schema and should support tests around branch-agnostic resource pools.
1184-1220: address_schema/address_data fixtures are consistent with each other.The address schema defines optional components and a read-only computed field, and the data fixture mirrors that layout with property dicts; shapes and types line up correctly.
1269-1425: device_schema/device_data fixtures comprehensively model a complex device with relationships.The schema for
InfraDeviceand the correspondingdevice_datapayload cover a wide range of relationship/property combinations (site, status, role, ASN, tags, primary_address, platform), which is excellent for deep integration tests; structure is coherent throughout.
1429-1475: artifact_definition_schema/artifact_definition_data fixtures look aligned with artifact-definition behavior.Schema attributes and the artifact definition data (including repository-sourced property metadata) reflect how artifact definitions are modeled elsewhere and should work well with the artifact-related client methods.
1478-1550: mock_branches_list_query and mock_repositories_query_no_pagination fixtures fit the new branch/report use cases.The branch list payload includes fields like
sync_with_git,has_schema_changes, andstatus, which match the new branch-report logic, and the repository mocks differentiate main vs cr1234 branches cleanly.
1552-1901: Repository and CoreNode mock query fixtures are internally consistent and reusable.All the
mock_query_repository_*andmock_query_corenode_*fixtures consistently usematch_headerstrackers andis_reusable=True, with payloads that match the GraphQL schemas; they should work well with the diff/branch/report flows exercised in this PR.
1903-2193: Artifact REST/GraphQL fixtures are detailed and match expected artifact lifecycle flows.The
mock_rest_api_artifact_fetchandmock_rest_api_artifact_generatefixtures combine schema loads, GraphQL artifact listings, and storage/REST calls with realistic content and property metadata; shapes look correct and cohesive.
2195-2217: Schema dropdown/enum mutation fixtures are minimal but sufficient.The four mutation fixtures returning
{"ok": True}are straightforward and reusable; no inconsistencies seen.
2219-2243: mock_query_mutation_location_create_failed correctly models a two-step success/failure scenario.First returning a successful create, then an error payload on the same URL regex, is a good way to exercise duplicate-handling code; structure of the error entry matches typical GraphQL error shapes.
2240-2250: Infrahub version/user fixtures are simple and coherent.Mocking
InfrahubInfo.versionand the user profile via fixtures directory provides stable inputs; both are small and well-formed.
254-269: GraphQL query string fixtures (query_01–query_introspection) remain unchanged semantically; returning raw triple-quoted strings is fine.The refactor to return multi-line string literals directly has no behavioral impact and keeps these fixtures easy to read.
2571-2602: Batch location query fixtures are structured appropriately for paginated testing.Looping over page fixture files and attaching them with
match_headerstrackers accurately simulates page-wise GraphQL responses; no issues detected.
2604-2682: Task query fixtures cover multiple pagination and emptiness scenarios.The various
mock_query_tasks_*fixtures pull JSON from fixtures_dir and attach them with distinct trackers, providing good coverage for empty, multi-page, and variant task lists; configuration looks consistent.
2684-2709: nested_device_with_interfaces_schema fixture is a sensible addition for deep nesting tests.The new schema defining a
DevicewithInfraInterfaceL3components via theinterfacesrelationship matches existing naming/identifier conventions (device__interface) and should support recursive/branching tests cleanly.infrahub_sdk/pytest_plugin/plugin.py (1)
92-95: Set-based suffix check is a clean, idiomatic improvement.Using
{".yml", ".yaml"}for the suffix membership inpytest_collect_fileis equivalent behavior with slightly clearer intent and better constant-time membership.tests/unit/sdk/test_yaml.py (1)
12-20: Renaming dir → test_data_dir clarifies intent and keeps the assertion accurate.The constructed
full_pathand expected error message now both referencetest_data_dir, avoiding the builtin name and matching howFileNotValidErrorreports missing paths.infrahub_sdk/pytest_plugin/models.py (1)
58-58: LGTM! Good use of set literal for membership checks.Using a set literal for membership tests is more idiomatic and slightly more efficient than a tuple, even though the performance difference is negligible for a 2-element collection.
tests/unit/sdk/graphql/conftest.py (1)
18-25: LGTM! Explicit return types improve type safety.The addition of explicit return type annotations and direct returns improve type safety and code clarity, aligning with the mypy strict configuration guideline.
tests/unit/sdk/graphql/test_plugin.py (1)
38-38: LGTM! Return type annotations added for test functions.Adding explicit
-> Nonereturn type annotations to test functions improves type safety and aligns with the mypy strict configuration guideline.Also applies to: 51-53
infrahub_sdk/ctl/graphql.py (2)
111-111: LGTM! Explicit UTF-8 encoding is best practice.Specifying UTF-8 encoding explicitly ensures consistent behavior across platforms and makes the encoding choice clear.
183-183: LGTM! More idiomatic dict iteration.Iterating directly over a dictionary is more idiomatic and slightly more efficient than iterating over
.keys().infrahub_sdk/schema/main.py (1)
6-6: LGTM! Type system modernization using Python 3.10+ syntax.The migration from
Unionto the pipe operator (|) for type unions is more concise and aligns with modern Python type annotation best practices.Also applies to: 14-14
tests/conftest.py (1)
3-3: LGTM! Explicit Generator type annotations improve type safety.Adding explicit
Generatortype annotations to fixtures improves type safety and aligns with the mypy strict configuration guideline. Usingcollections.abc.Generatoris the recommended approach for Python 3.9+.Also applies to: 15-15, 30-30
infrahub_sdk/node/relationship.py (1)
84-91: Remove this review comment - the code correctly handles properties conditionally.The review comment's core premise is incorrect. The
propertiesfield is not unconditionally added todata["edges"]. Looking at the implementation:
- Line 85:
if property:— conditional check- Lines 86-89: populate properties dict only when condition is true
- Line 90:
data["edges"]["properties"] = properties— this assignment is inside the if blockWhen
property=False(the default), the properties field is never added to the query structure, which is the correct behavior. The same pattern is consistently implemented inrelated_node.py(line 173). There are no GraphQL validation issues—empty properties dictionaries are not added to queries.Likely an incorrect or invalid review comment.
infrahub_sdk/node/related_node.py (1)
67-67: Update test fixtures with "_relation__updated_at" to use new format and add test coverage for updated_at field on RelatedNode.The code change removes the fallback to
_relation__updated_at, replacing it with a lookup forupdated_atinproperties_data. However:
- Test fixtures at lines 1805, 1813, 1860 still use the old
_relation__updated_atformat in related node data- Unlike other metadata properties (which fall back to
_relation__{prop}),updated_athas no fallback to_relation__updated_at- No tests verify that
RelatedNode.updated_atis correctly populated from actual dataThis breaks backward compatibility if GraphQL responses still provide
_relation__updated_at. Either:
- Verify GraphQL schema was updated to provide
updated_atdirectly in related node objects, or- Add fallback logic:
data.get("updated_at", properties_data.get("updated_at", properties_data.get("_relation__updated_at")))- Update test fixtures and add a test case that verifies
updated_atgets set correctly on RelatedNode initializationinfrahub_sdk/query_groups.py (2)
171-171: LGTM!The explicit
list()conversion correctly aligns with the declared typelist[str] | Noneon line 22, and the more specific# type: ignore[union-attr]is better practice. The async and sync implementations are consistent.
265-265: Consistent with async implementation.The sync implementation correctly mirrors the async version, following the dual implementation pattern.
infrahub_sdk/operation.py (1)
68-68: LGTM!Idiomatic simplification - direct membership testing on a dict is equivalent to checking
.keys().infrahub_sdk/schema/repository.py (1)
18-21: LGTM!Clean type annotation modernization using Python 3.10+ union syntax. The
from __future__ import annotationsimport ensures this works correctly, and placement underTYPE_CHECKINGmeans no runtime impact.infrahub_sdk/diff.py (2)
39-50: LGTM!Well-structured TypedDict with appropriate fields for aggregating diff metrics. The use of
NotRequiredfor the optionalnamefield is correct.
144-204: LGTM!Clean implementation using the
Queryclass for structured query construction. The variable types (datetime | Nonefor time parameters) appropriately map to GraphQL DateTime types.tests/integration/test_infrahub_client.py (2)
60-61: LGTM!Idiomatic simplification - direct membership testing on a dict is cleaner and equivalent to checking
.keys().
109-109: LGTM!Using a set literal for membership testing is more appropriate when order doesn't matter and conveys the intent more clearly.
infrahub_sdk/ctl/cli_commands.py (1)
9-11: LGTM!The import relocation of
Callablefromtypingtocollections.abcis a modern Python best practice. This aligns with the broader type modernization effort across the codebase.infrahub_sdk/node/attribute.py (2)
4-5: LGTM!Import modernization aligns with Python best practices.
29-57: LGTM!The data initialization logic is clean and handles both raw values and dictionary inputs correctly. The property initialization using
data.get()without explicitNonedefaults is appropriate sincedict.get()returnsNoneby default.tests/unit/sdk/test_node.py (3)
27-31: LGTM!Using set literals
{"hfid", "hfid_str"}instead of tuples/lists for membership checks is more efficient (O(1) lookup).
1453-1454: LGTM!Using
next(iter(...))is more memory-efficient thanlist(...)[0]as it avoids creating an intermediate list.
2458-2667: LGTM!Comprehensive test for the new recursive relationship processing feature. The test properly validates:
- Non-recursive mode processes only the first level (2 interfaces)
- Recursive mode processes all levels (2 interfaces + 3 IP addresses)
- Both async and sync clients are tested via parametrization
infrahub_sdk/ctl/branch.py (3)
27-33: LGTM!The
format_timestampfunction properly handles ISO format conversion with a fallback to the original string on parsing errors.
36-60: LGTM!The function properly handles the API call with appropriate error propagation and defensive checks on the response structure.
190-299: New branch report command is well-structured.The command follows Typer conventions and provides useful branch status information. The validation for the main branch and the Rich table formatting are appropriate.
infrahub_sdk/node/node.py (4)
287-292: LGTM!Iterating directly over the dictionary (
for item_key in original_data[item]) is more Pythonic than iterating over.keys(). No behavioral change.
744-744: LGTM!Using a set literal for membership testing provides O(1) lookup performance.
892-945: LGTM!The recursive relationship processing is well-implemented:
- The
recursiveparameter defaults toFalsefor backward compatibility- The same
related_nodeslist is passed down to accumulate all discovered nodes- The implementation correctly handles both
RelatedNode(cardinality one) andRelationshipManager(cardinality many) relationships
1539-1592: LGTM!The sync version of
_process_relationshipscorrectly mirrors the async implementation, maintaining the dual client pattern as per coding guidelines.tests/unit/sdk/test_schema.py (1)
458-458: LGTM! Type annotation added.The explicit return type annotation aligns with the broader type system modernization across the codebase.
infrahub_sdk/client.py (4)
8-8: LGTM! Import additions support new functionality.The
Callableimport fromcollections.abcfollows modern Python conventions, and the expanded diff imports (DiffTreeData,get_diff_tree_query) are properly used in the newget_diff_treemethods.Also applies to: 37-37
302-302: Good optimization: tuple literals replaced with set literals for membership checks.Converting membership checks from tuples to sets is a best practice improvement. While the performance gain is negligible for such small collections, it improves consistency across the codebase.
Also applies to: 959-959, 973-973, 1211-1211, 1878-1878, 1892-1892, 2507-2507
1285-1341: LGTM! New diff tree API method with proper validation.The async
get_diff_treemethod correctly:
- Validates temporal ordering (
from_time <= to_time)- Converts datetime objects to ISO format for the GraphQL query
- Handles
Noneresponses gracefully- Maps response data to
DiffTreeDatastructure usingdiff_tree_node_to_node_diff- Uses
.render()on the Query object (correct forexecute_graphql)
2581-2637: LGTM! Sync version correctly mirrors async implementation.The synchronous
get_diff_treemethod properly implements the dual client pattern with identical logic, validation, and error handling as its async counterpart.infrahub_sdk/schema/__init__.py (3)
10-10: LGTM! Type system modernized to Python 3.10+ standards.The changes properly adopt:
- Explicit
TypeAliasannotations per PEP 613- Union syntax using
|operator per PEP 604InfrahubNodeTypescorrectly placed inTYPE_CHECKINGblock to avoid circular importsThese align with the codebase's migration to Python 3.10+ (dropping 3.9 support).
Also applies to: 48-48, 86-90
125-125: Minor optimization: removed redundant.keys()call.Iterating directly over a dict is idiomatic Python and slightly more efficient than explicitly calling
.keys().
196-201: Good optimization: status code check uses set literal.Consistent with other membership check optimizations across the codebase. Set membership is more efficient and semantically appropriate for this use case.
infrahub_sdk/spec/range_expansion.py (1)
70-72:strict=Falseonzipis correct and properly scoped to Python ≥3.10The
strictkeyword argument was added tozip()in Python 3.10 with a default value ofFalse. The code change at line 72 correctly useszip(it, it, strict=False)to preserve the prior behavior while making intent explicit. The packaging metadata (pyproject.toml) declaresrequires-python = ">=3.10,<3.14", which fully aligns with this implementation and eliminates any compatibility concerns.pyproject.toml (4)
3-3: Python 3.10+ baseline is consistently applied.Version bump to 1.16.0b0 and requires-python constraint are aligned with classifier updates. The tomli conditional (line 28) correctly uses
python_version<'3.11'to match the new baseline.Also applies to: 10-10, 11-18
225-234: New flake8-builtins configuration documents intentional shadowing.The ignorelist appropriately documents common builtin shadowing cases with a note for future review. This aligns with the PR's modernization goals and provides visibility for technical debt.
253-256: Clarify the scope and intent of ASYNC240 allowance for ctl module.Line 255 introduces ASYNC240 (async functions should not use pathlib.Path methods) without a "Review and update..." comment header like other entries. Confirm whether this exemption is a temporary workaround or a permanent design decision for the ctl module.
84-84: Verify Ruff 0.14.5 compatibility by testing locally before merge.The upgrade to Ruff 0.14.5 introduces breaking changes and new rules that will be activated by your existing
preview = trueconfiguration:
- Breaking changes: target-version inference behavior (0.11.0), Python 3.14 default support (0.14.0)
- New stabilized rules: SIM113, DOC102/DOC502, RUF065, FURB101, and formatter behavior changes will activate
- CI dependency: Your CI runs
ruff checkandruff format --checkon every Python file change, which will fail if new violations are introducedTest locally before merge:
uv sync --group lint uv run ruff check . uv run ruff format --check --diff .Address any new violations or adjust linting rules to exclude them before proceeding.
.github/workflows/ci.yml (3)
227-231: Python matrix correctly aligns with project baseline.The test matrix 3.10–3.13 properly reflects the updated requires-python constraint. All workflow jobs consistently use the new baseline.
72-93: All verifications passed—no issues found.The prepare-environment workflow properly exports UV_VERSION, the python-lint job correctly depends on it, and setup-uv@v7 correctly receives the version parameter. The workflow structure is sound.
248-251: No action required.Astral-sh/setup-uv v7 supports the
python-versioninput parameter, which sets UV_PYTHON for the workflow. The code on line 251 is correct.
wvandeun
force-pushed
the
wvd-20251114-infp388-branch-cleanup-mechanism
branch
from
dd549f6 to
101f45a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
infrahub_sdk/client.py (1)
1285-1339: Asyncget_diff_treeimplementation looks correct and consistentThe method validates the time range, builds variables, calls
get_diff_tree_query().render()withvariables=input_data, gracefully handlesDiffTree is None, and converts nodes viadiff_tree_node_to_node_diffinto a well-shapedDiffTreeData. This is consistent withget_diff_summaryand the diff module types.If duplication between async/sync ever becomes an issue, you could factor the common “build
input_data+ mapdiff_tree→DiffTreeData” into a small private helper and call it from both clients, but it’s not required for correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrahub_sdk/client.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/client.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/client.py: UseInfrahubClient(async) andInfrahubClientSync(sync) with identical interfaces following the Dual Client Pattern
Use HTTPX-based transport with proxy support (single proxy or HTTP/HTTPS mounts) and API token/JWT authentication with automatic refresh
Files:
infrahub_sdk/client.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All async operations must have corresponding sync implementations following the dual implementation pattern
Use mypy with strict configuration for type checking in Python files
Files:
infrahub_sdk/client.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: wvandeun
Repo: opsmill/infrahub-sdk-python PR: 637
File: infrahub_sdk/ctl/branch.py:290-290
Timestamp: 2025-11-25T13:29:23.043Z
Learning: In infrahub_sdk/ctl/branch.py, the report command uses pc.created_by.updated_at to display "Created at" for proposed changes as a workaround because the SDK doesn't have easy access to the creation time of the proposed change. This will be replaced with proper object-level metadata implementation in version 1.7 of Infrahub.
📚 Learning: 2025-11-25T07:18:58.135Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T07:18:58.135Z
Learning: Applies to **/client.py : Use `InfrahubClient` (async) and `InfrahubClientSync` (sync) with identical interfaces following the Dual Client Pattern
Applied to files:
infrahub_sdk/client.py
🧬 Code graph analysis (1)
infrahub_sdk/client.py (1)
infrahub_sdk/diff.py (5)
DiffTreeData(39-49)NodeDiff(11-17)diff_tree_node_to_node_diff(91-141)get_diff_summary_query(52-88)get_diff_tree_query(144-204)
⏰ 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). (6)
- GitHub Check: unit-tests (3.13)
- GitHub Check: documentation
- GitHub Check: unit-tests (3.11)
- GitHub Check: unit-tests (3.12)
- GitHub Check: unit-tests (3.10)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
infrahub_sdk/client.py (2)
37-37: Diff-related imports are correct and fully usedAll newly imported symbols (
DiffTreeData,NodeDiff,diff_tree_node_to_node_diff,get_diff_summary_query,get_diff_tree_query) are used below and match the new diff APIs.
2579-2633: Syncget_diff_treemirrors async API and preserves dual-client patternThe sync implementation is interface-identical to the async version and mirrors its logic (validation, variable building, execution, and
DiffTreeDataconstruction), which keepsInfrahubClient/InfrahubClientSyncaligned as per the dual client pattern.
…114-infp388-branch-cleanup-mechanism # Conflicts: # infrahub_sdk/node/constants.py # tests/unit/sdk/test_node.py
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/sdk/test_node.py (1)
2417-2625: Well-structured test for recursive relationship processing.This test thoroughly validates the
_process_relationshipsbehavior with deep nesting across both async and sync clients. The test data and assertions clearly demonstrate the difference between recursive and non-recursive processing.Optional improvements to consider:
- The schema setup could be extracted into a fixture to improve reusability and reduce test length.
- The nested data structure (~60 lines) could be moved to a fixture or helper function.
These are minor readability suggestions—the current implementation is correct and follows existing patterns in the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
infrahub_sdk/node/constants.py(1 hunks)tests/unit/sdk/conftest.py(6 hunks)tests/unit/sdk/test_node.py(37 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/unit/sdk/conftest.py
🚧 Files skipped from review as they are similar to previous changes (1)
- infrahub_sdk/node/constants.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All async operations must have corresponding sync implementations following the dual implementation pattern
Use mypy with strict configuration for type checking in Python files
Files:
tests/unit/sdk/test_node.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place unit tests in
tests/unit/directory for testing individual components in isolation
Files:
tests/unit/sdk/test_node.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: wvandeun
Repo: opsmill/infrahub-sdk-python PR: 637
File: infrahub_sdk/ctl/branch.py:290-290
Timestamp: 2025-11-25T13:29:23.062Z
Learning: In infrahub_sdk/ctl/branch.py, the report command uses pc.created_by.updated_at to display "Created at" for proposed changes as a workaround because the SDK doesn't have easy access to the creation time of the proposed change. This will be replaced with proper object-level metadata implementation in version 1.7 of Infrahub.
⏰ 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). (7)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: unit-tests (3.13)
- GitHub Check: unit-tests (3.11)
- GitHub Check: unit-tests (3.12)
- GitHub Check: unit-tests (3.10)
- GitHub Check: documentation
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
tests/unit/sdk/test_node.py (2)
329-329: LGTM: Test expectations updated to includeupdated_atfield.The mechanical addition of
"updated_at": Nonethroughout query data expectations is consistent and aligns with the broader changes in this PR to trackupdated_atacross properties and relationship properties.Also applies to: 346-346, 363-363, 379-379, 456-456, 465-465, 474-474, 490-490, 571-571, 588-588, 605-605, 621-621, 642-642, 659-659, 742-742, 751-751, 763-763, 775-775, 783-783, 792-792, 903-903, 921-921, 944-944, 962-962, 982-982, 1000-1000, 1115-1115, 1132-1132, 1149-1149, 1165-1165, 1189-1189, 1298-1298, 1315-1315
1706-1717: LGTM: Input data expectations correctly includeupdated_attimestamps.The test expectations properly validate that
updated_attimestamps are preserved when generating input data for node updates. This ensures that the original timestamp values are carried through from the GraphQL response data.Also applies to: 1766-1766, 1769-1769, 1776-1776, 1786-1786, 1823-1823, 1873-1884
|
|
||
| return node_diffs | ||
|
|
||
| async def get_diff_tree( |
There was a problem hiding this comment.
Do you see a value from an SDK point of view to have this as a method directly under the client? I don't have a strong objection to it but the client file is already massive as it is and at some point I'd like to break it apart into smaller components. Don't know if this command would remain in the same place at that point. Not really ready for that refactoring just now. But if we don't see a clear use case to have this within the SDK itself I'd suggest that we start out by just having it in the CTL.
| num_added=diff_tree.get("num_added") or 0, | ||
| num_updated=diff_tree.get("num_updated") or 0, | ||
| num_removed=diff_tree.get("num_removed") or 0, | ||
| num_conflicts=diff_tree.get("num_conflicts") or 0, |
There was a problem hiding this comment.
Looking at this I think it comes from the fact that the GraphQL query itself has an incorrect return format. I.e. all of these should be required:
I.e. it should never return a null value like it looks like the query can do now. Based on the issue in Infrahub I think your approach would be the way to go. But we can create a followup task for it later to clean this up.
| dt = datetime.fromisoformat(timestamp.replace("Z", "+00:00")) | ||
| return dt.strftime("%Y-%m-%d %H:%M:%S") | ||
| except (ValueError, AttributeError): | ||
| return timestamp |
There was a problem hiding this comment.
This looks like a safeguard incase the response from Infrahub is invalid. With the docstring it's not clear that an incorrect value could be returned.
Is the issue here that we ignore typing somewhere and timestamp can be None?
| proposal_table.add_row("Created by", pc.created_by.peer.name.value) # type: ignore[union-attr] | ||
| proposal_table.add_row("Created at", format_timestamp(str(pc.created_by.updated_at))) # type: ignore[union-attr] | ||
| proposal_table.add_row("Approvals", str(len(pc.approved_by.peers))) # type: ignore[union-attr] | ||
| proposal_table.add_row("Rejections", str(len(pc.rejected_by.peers))) # type: ignore[union-attr] |
There was a problem hiding this comment.
We should avoid adding new ignore statements for typing. I'm guessing that these could be removed if we instead used the protocol definition and not the string "CoreProposedChange" when querying for these objects.
| proposal_table.add_row("Name", pc.name.value) # type: ignore[union-attr] | ||
| proposal_table.add_row("State", str(pc.state.value)) # type: ignore[union-attr] | ||
| proposal_table.add_row("Is draft", "Yes" if pc.is_draft.value else "No") # type: ignore[union-attr] | ||
| proposal_table.add_row("Created by", pc.created_by.peer.name.value) # type: ignore[union-attr] |
There was a problem hiding this comment.
Based on the changes for immutable metadata in Infrahub 1.7, I'd consider waiting to include the creator of the PC.
| for prop_name in PROPERTIES_OBJECT: | ||
| properties[prop_name] = {"id": None, "display_label": None, "__typename": None} | ||
|
|
||
| if properties: |
There was a problem hiding this comment.
Was this something that was required?
| method="GET", | ||
| url="http://mock/api/diff/files?branch=test-branch", | ||
| status_code=404, | ||
| ) |
There was a problem hiding this comment.
I think this test is easier to read than the ones above, if we look at a fixture like mock_git_files_empty_response which is only used once and is so short I'd recommend having it inline within the test body as it's easier to see at a glance what's going on. If we know that we're going to use the fixture in multiple locations I think that's fine, but if they only are used once we also run the risk of leaving the fixture in place if the test is ever deleted.
|
|
||
| def test_branch_report_command_without_proposed_change( | ||
| mock_branch_report_command: HTTPXMock, | ||
| mock_schema_query_05, # type: ignore[misc] |
There was a problem hiding this comment.
Looks like this is a HTTPXMock class to, so it should be fine to add that and remove the ignore statement?
|
|
||
| # Verify it's a Query object | ||
| assert hasattr(query, "render") | ||
| assert hasattr(query, "name") |
There was a problem hiding this comment.
Typing should catch this and if the name or render function doesn't exist the asserts just below would fail. So we can remove the hasattr checks.
| else: | ||
| branch_table.add_row("Git files changed", "N/A") | ||
|
|
||
| branch_table.add_row("Has schema changes", "Yes" if branch.has_schema_changes else "No") |
There was a problem hiding this comment.
I think this part could potentially be a bit misleading for stale branches. The only thing this flag does is to indicate if the schema differs compared to the default branch. This field is probably incorrectly named within Infrahub and should be called something like "schema in sync with default branch". The problem here would be if the default branch has moved on with other changes.
Could be that this naming would remain consistent with what we have within Infrahub, but I think the proper way to do this would be to do a diff of the schema as well. This might also be enough for an initial round.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.