fix(infrahub adapter): stop stringifying non-IP attribute values#130
Open
PhillSimonds wants to merge 6 commits into
Open
fix(infrahub adapter): stop stringifying non-IP attribute values#130PhillSimonds wants to merge 6 commits into
PhillSimonds wants to merge 6 commits into
Conversation
InfrahubAdapter.infrahub_node_to_diffsync() was passing every non-string attribute value through str(). For attributes of kind: List, that turned the real list (e.g. []) into the string literal "[]", which then failed Pydantic validation on DiffSync model fields typed as list[str]. The same bug affected kind: Number (int → string) and kind: Boolean (bool → string), though Pydantic 2 sometimes coerces those back. Restrict the stringification to the originally-intended ipaddress types. The Infrahub SDK already returns DateTime values as ISO-8601 strings, so no extra coercion is needed for that kind either. Verified end-to-end against Infrahub 1.9.6 with a minimal reproduction (see issue): a Bookmark node with a List-kind `tags` attribute now round-trips through the adapter as a real list and constructs the DiffSync model without error. Closes #129 🤖 Generated with Claude Code (https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughInfrahubAdapter.infrahub_node_to_diffsync now stringifies only IP SDK types (IPv4/IPv6 Interface and Network). Other non-string attribute values (lists, numbers, booleans, datetimes, and None) are preserved and passed through unchanged. The PR also adds unit and integration tests covering these behaviors, documents an 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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-sync with
|
| Latest commit: |
60f5cd4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://adb35c16.infrahub-sync.pages.dev |
| Branch Preview URL: | https://ps-fix-infrahub-adapter-list.infrahub-sync.pages.dev |
…ahub_node_to_diffsync Adds the first test coverage for InfrahubAdapter.infrahub_node_to_diffsync. Tests bypass network setup by constructing the adapter via __new__ and supplying only self.config plus a node-like fixture with one attribute per kind. Relationship handling is out of scope here. Coverage: - Text, Number, Boolean, List (incl. empty), JSON-as-dict, DateTime-as-str, None — must pass through unchanged with their original Python type. - IPv4Interface, IPv6Interface, IPv4Network, IPv6Network — must be stringified (parametrised). - Fields absent from schema_mapping are skipped (has_field filter). - Mixed-kind round-trip on a realistic certificate-shaped node. Six of these tests fail on the pre-fix code (List, empty List, Number, Boolean, dict, mixed) and pass on the fixed code. The IP-stringification tests pass on both, locking in the intended behavior. 🤖 Generated with Claude Code (https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dict shape The previous tests checked the dict returned by infrahub_node_to_diffsync. That misses the contract that actually matters in practice: the dict gets fed straight into DiffSyncModel(**data) by InfrahubAdapter.load, and Pydantic validates the kwargs against the model's typed fields. The original bug surfaced there — not on the dict shape. Add two end-to-end tests that define a Pydantic-typed DiffSync model shaped like the F5 CertificateCertificate (the model that triggered the report) and feed the adapter's output into it directly. Both tests fail on the pre-fix code with the user's original error (`Input should be a valid list, input_value='[]', input_type=str`) and pass after the fix. Notably, the failures Pydantic raises are scoped to list-typed and dict- typed fields only — Pydantic 2 silently coerces "443" to int and "True" to bool, so the bug never manifested for Number/Boolean kinds in practice. The broad fix is correctness-not-regression for those kinds. 🤖 Generated with Claude Code (https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ests in CI
Adds two pieces of test infrastructure that the repo previously lacked:
1. Live-SDK integration test (`tests/integration/`)
--------------------------------------------------
The unit tests use lightweight `FakeNode` stand-ins; they catch the
behavior under test but can't catch drift between the fixture and the
real `InfrahubNodeSync` shape. This new integration test exercises
`InfrahubAdapter.infrahub_node_to_diffsync` against a live Infrahub:
applies a throwaway `TestAdapterProbe` schema with one attribute of
each kind (Text, List, Number, Boolean, DateTime, JSON, IPHost),
creates a node, runs the adapter, feeds the result into a Pydantic-
typed DiffSync model, asserts both value and type per field, and
tears down. Marked `@pytest.mark.integration` and skipped when
INFRAHUB_ADDRESS+INFRAHUB_API_TOKEN aren't set, so it's a no-op in
environments without an Infrahub.
Verified against an Infrahub 1.9.6 lab: passes on the fixed adapter
in ~13s, fails on the pre-fix adapter with the user-reported error.
2. CI plumbing (workflow-tests.yml + tasks/tests.py)
--------------------------------------------------
The repo had a `workflow-tests.yml` that did file-change detection
but skipped running any tests (the unit test job was commented out
with a TODO). And `tasks/tests.py` had empty pass-through stubs.
Neither of those would have gated the bug being fixed in this PR.
- `tasks.tests_unit` now runs `pytest -m "not integration"`.
- `tasks.tests_integration` runs `pytest -m integration` (opt-in).
- Registered the `integration` marker in pyproject.toml.
- Uncommented and rewrote the unit-tests job to install via uv and
invoke `tests.tests-unit` across Python 3.10–3.13.
- Integration tests are not gated by CI yet — that would need an
Infrahub service container or compose stack and is left as a
follow-up. The integration test runs locally / in any environment
that points at a real Infrahub.
🤖 Generated with Claude Code (https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
.github/workflows/workflow-tests.yml (1)
48-49: 💤 Low valueOptional: disable credential persistence on checkout. This job only reads code and runs tests, so it doesn't need the token persisted in
.git/config. Settingpersist-credentials: falsereduces the credential-leak surface flagged by zizmor (artipacked).🔒 Proposed hardening
- name: "Check out repository code" uses: "actions/checkout@v5" + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/workflow-tests.yml around lines 48 - 49, The checkout step "Check out repository code" currently uses actions/checkout@v5 without disabling credential persistence; update that step to add persist-credentials: false so the workflow does not store the GITHUB_TOKEN in .git/config (i.e., modify the step for actions/checkout@v5 to include the persist-credentials: false input to reduce credential leak surface).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/adapters/test_infrahub_node_to_diffsync.py`:
- Line 156: Replace the literal 443 in the test assertions with a module-level
constant to satisfy Ruff PLR2004: add a top-level constant like HTTPS_PORT = 443
in tests/adapters/test_infrahub_node_to_diffsync.py and change the assertions
that use the literal (e.g. the one checking data["port"] at the spots mentioned)
to compare against HTTPS_PORT; ensure both occurrences (the one at line ~156 and
the one at line ~342) are updated to use HTTPS_PORT.
- Around line 15-29: The top import block in
tests/adapters/test_infrahub_node_to_diffsync.py is misformatted and triggers
Ruff I001; reorder and group the imports into standard library (from __future__
and ipaddress), third-party (dataclasses, typing, pytest), then local package
imports (infrahub_sync and its symbols and infrahub adapter), and alphabetize
within groups as Ruff expects, or simply run `ruff check --fix` / `ruff format`
to automatically reformat the import block so the imports for
SchemaMappingField, SchemaMappingModel, SyncAdapter, SyncConfig, and
InfrahubAdapter are in the correctly grouped/ordered import section.
- Line 204: The test function signature test_ip_types_are_stringified(raw: Any,
expected: str) triggers Ruff ANN401; suppress it locally by appending a per-line
noqa for ANN401 to the function definition (e.g., add "# noqa: ANN401" to the
def line for test_ip_types_are_stringified) so the heterogeneous IP parameter
keeps its Any annotation without weakening project-wide typing rules.
In `@tests/integration/test_infrahub_node_to_diffsync_integration.py`:
- Line 140: Ruff ANN401 is triggered by using typing.Any in _graphql_literal and
the _make_infrahub_client return type; add a local suppression by appending "#
noqa: ANN401" to the definitions for _graphql_literal (the "def
_graphql_literal(value: Any) -> str:" line) and the _make_infrahub_client
signature (the "def _make_infrahub_client(...)" return annotation around line
264) so the linter ignores the ANN401 warning for these legitimate Any uses.
- Line 226: The inline stub for adapter.store uses a lambda with unused
parameters causing Ruff ARG005; update the lambda in the dynamic type assignment
(currently type("S", (), {"get": lambda self, **k: None})()) to rename unused
parameters with leading underscores (e.g., **_k or _self as appropriate) so the
linter recognizes them as intentionally unused while preserving behavior of the
get method on the S stub.
- Line 156: The f-string used when building items uses single quotes which
triggers Ruff Q000; change the f-string in the generator expression to use
double quotes so it reads f"{k}: {_graphql_literal(v)}" (update the expression
in the assignment to items where the generator uses f'{k}:
{_graphql_literal(v)}'), leaving the surrounding join and value.items() logic
unchanged.
- Around line 99-101: The inline f-string used directly in the raise statements
causes Ruff TRY003/EM102; assign the formatted message to a variable first and
raise using that variable. For the block that checks body.get("errors") change
to: err_msg = f"GraphQL errors: {body['errors']}" then raise
RuntimeError(err_msg), and similarly for the other site that raises TypeError
create a message variable (e.g., err_msg = f"...") and raise TypeError(err_msg);
reference the existing body variable and the raise RuntimeError / raise
TypeError sites when making the edits.
- Line 27: The top-level import of Iterator causes Ruff TC003; move the import
into a typing-only guard so it isn’t imported at runtime: remove "from
collections.abc import Iterator" from the module-level imports, add "from typing
import TYPE_CHECKING" near the imports, and under "if TYPE_CHECKING:" add "from
collections.abc import Iterator" so the fixture return annotation can still
reference Iterator without importing it at runtime (target the Iterator symbol
and the fixture that uses it).
---
Nitpick comments:
In @.github/workflows/workflow-tests.yml:
- Around line 48-49: The checkout step "Check out repository code" currently
uses actions/checkout@v5 without disabling credential persistence; update that
step to add persist-credentials: false so the workflow does not store the
GITHUB_TOKEN in .git/config (i.e., modify the step for actions/checkout@v5 to
include the persist-credentials: false input to reduce credential leak surface).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c0383772-d6ee-4229-9ace-0c683a23622b
📒 Files selected for processing (6)
.github/workflows/workflow-tests.ymlpyproject.tomltasks/tests.pytests/adapters/test_infrahub_node_to_diffsync.pytests/integration/__init__.pytests/integration/test_infrahub_node_to_diffsync_integration.py
✅ Files skipped from review due to trivial changes (1)
- tests/integration/init.py
Comment on lines
+15
to
+29
| from __future__ import annotations | ||
|
|
||
| import ipaddress | ||
| from dataclasses import dataclass, field | ||
| from typing import Any | ||
|
|
||
| import pytest | ||
|
|
||
| from infrahub_sync import ( | ||
| SchemaMappingField, | ||
| SchemaMappingModel, | ||
| SyncAdapter, | ||
| SyncConfig, | ||
| ) | ||
| from infrahub_sync.adapters.infrahub import InfrahubAdapter |
Contributor
There was a problem hiding this comment.
Ruff I001 blocks CI — import block needs reformatting. The lint job (which this PR activates) fails on the top import block. Run ruff check --fix / ruff format to sort it.
🧰 Tools
🪛 GitHub Actions: Pull Request on main/stable / 4_linter _ python-lint (3.10).txt
[error] 15-15: Ruff I001: Import block is un-sorted or un-formatted. Organize imports.
🪛 GitHub Actions: Pull Request on main/stable / 5_linter _ python-lint (3.12).txt
[error] 15-29: ruff (I001) Import block is un-sorted or un-formatted. Organize imports.
🪛 GitHub Actions: Pull Request on main/stable / 6_linter _ python-lint (3.11).txt
[error] 15-29: ruff (I001) Import block is un-sorted or un-formatted. Organize imports.
🪛 GitHub Actions: Pull Request on main/stable / linter _ python-lint (3.10)
[error] 15-29: Ruff I001 Import block is un-sorted or un-formatted. Organize imports.
🪛 GitHub Actions: Pull Request on main/stable / linter _ python-lint (3.11)
[error] 15-28: Ruff I001 Import block is un-sorted or un-formatted. Fix: Organize imports.
🪛 GitHub Actions: Pull Request on main/stable / linter _ python-lint (3.12)
[error] 15-29: ruff check failed (I001). Import block is un-sorted or un-formatted. Help: Organize imports.
🪛 GitHub Actions: Pull Request on main/stable / linter _ python-lint (3.13)
[error] 15-15: Ruff (I001) import block is un-sorted or un-formatted. Help: Organize imports.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/adapters/test_infrahub_node_to_diffsync.py` around lines 15 - 29, The
top import block in tests/adapters/test_infrahub_node_to_diffsync.py is
misformatted and triggers Ruff I001; reorder and group the imports into standard
library (from __future__ and ipaddress), third-party (dataclasses, typing,
pytest), then local package imports (infrahub_sync and its symbols and infrahub
adapter), and alphabetize within groups as Ruff expects, or simply run `ruff
check --fix` / `ruff format` to automatically reformat the import block so the
imports for SchemaMappingField, SchemaMappingModel, SyncAdapter, SyncConfig, and
InfrahubAdapter are in the correctly grouped/ordered import section.
Drop bug-history phrasing ("original bug", "regression guard", "prior to
this fix", etc.) from test docstrings and inline comments. Tests describe
what they ensure about current behavior — Pydantic strict-mode rejecting
stringified lists/dicts, value+type contracts at the dict boundary, etc.
— rather than narrating how the code got here.
🤖 Generated with Claude Code (https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI runs ruff (check + format) and ty across Python 3.10–3.13. The new
test files tripped 16 ruff errors and 14 ty diagnostics; fix them:
- I001: sort imports.
- PLR2004: extract magic literal 443 to a module-level HTTPS_PORT.
- ANN401: scoped `# noqa: ANN401` on heterogeneous-by-design typing.Any
params (parametrised IP types, GraphQL literal renderer, SDK
client factory).
- ERA001: rewrite tests/integration/__init__.py comment as a module
docstring (the `pytest tests/integration -m integration`
example was getting flagged as commented-out code).
- TC003: move `from collections.abc import Iterator` into TYPE_CHECKING.
- TRY003 / EM102: assign exception messages to a `msg` local before
`raise`.
- Q000: double-quote inside f-string.
- ARG005: underscore-prefix unused lambda args in the store stub.
- ty invalid-argument-type: introduce `_serialise(adapter, node)` helper
that suppresses the FakeNode-vs-InfrahubNodeSync mismatch once,
instead of `# ty: ignore` on every call. The body still passes the
fake straight through — it duck-types the bits the method reads.
Tests still pass: 25/25 unit, 1/1 integration against a live Infrahub.
🤖 Generated with Claude Code (https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Tests
CI / Chores
Bug Fixes
InfrahubAdapter.infrahub_node_to_diffsync()was stringifying every non-string attribute value, not just theipaddresstypes it was meant to normalise. Lists became"[]", dicts became"{}", etc.str()'d; everything else passes through.InfrahubAdapter.loadactually exercises at runtime, and where the original failure surfaced.Closes #129.
Test plan
pytest tests/adapters/test_infrahub_node_to_diffsync.py— 17/17 pass on the fix.origin/main(pre-fix) — 8 fail (6 dict-shape + 2 Pydantic-model-construction), 9 pass.Input should be a valid list [type=list_type, input_value='[]', input_type=str].468109cduring unrelated lineage-source work; looked accidental).Practical impact of the broad fix
Worth noting for reviewers — when the model-construction tests run against the buggy code, Pydantic only flags
listanddictfields. It silently coerces"443"→intand"True"→bool, so:kind: List— was broken (this PR fixes it)kind: JSON— was broken (this PR fixes it)kind: Number— was "working" because Pydantic coerced; this PR makes it correctly-typed at the dict boundary tookind: Boolean— samekind: Text/DateTime/ IP types — unchangedSo the user-visible regression is contained to List and JSON kinds. The other type changes are correctness improvements rather than bug fixes.
🤖 Generated with Claude Code