Merge 'develop' into 'infrahub-develop' #507
Conversation
Use correct case for Infrahub
Send in the correct and expected types
Bumps [actions/checkout](https://github.com/actions/checkout) from 4 to 5. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v4...v5) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '5' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>
* document INFRAHUB_PROXY * vale * corrections * fix tabs * rabbit * sanity checking * Complete rework of client guide * Lint * Vale --------- Co-authored-by: Baptiste <[email protected]>
…p/actions/checkout-5 Bump actions/checkout from 4 to 5
Remove unnecessary call (rewrite as a literal)
Auto fix return type None for tests
Update packages in local lock file
Merge stable into develop with resolved conflicts
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## infrahub-develop #507 +/- ##
====================================================
- Coverage 75.85% 75.83% -0.02%
====================================================
Files 100 100
Lines 8874 9303 +429
Branches 1741 1826 +85
====================================================
+ Hits 6731 7055 +324
- Misses 1667 1756 +89
- Partials 476 492 +16
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
WalkthroughThis pull request updates CI workflows to use actions/checkout@v5 throughout. Documentation is expanded: a new CLAUDE.md is added; the Python SDK client guide is substantially rewritten; Docusaurus dependencies are bumped to 3.8.1; Vale sentence-case exceptions are adjusted. Minor code tweaks include stringifying response.url in decode_json errors and passing root_directory=str(directory) to GitRepoManager.get_branch. Linting changes enable Ruff rule C408. Numerous tests gain explicit -> None return annotations, with small assertion/coverage adjustments in CLI details, repository initialization, schema option errors, utils UUID validation, and a dict literal in topological sort. Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
group/ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (5)
infrahub_sdk/pytest_plugin/items/graphql_query.py (1)
31-33: Fix catch: httpx Response.json() won’t raise ujson.JSONDecodeError
response.json()uses the stdlib JSON decoder by default, so JSON parse failures won’t be caught byujson.JSONDecodeError. This can mask the original HTTP error by throwing during error formatting.Apply this minimal, robust fix:
- try: - response_content = ujson.dumps(excinfo.value.response.json(), indent=4) - except ujson.JSONDecodeError: - response_content = excinfo.value.response.text + try: + response_content = ujson.dumps(excinfo.value.response.json(), indent=4) + except Exception: + # If the body isn't valid JSON, fall back to raw text + response_content = excinfo.value.response.texttests/unit/ctl/test_branch_app.py (1)
15-25: Convert async fixtures to synchronous definitions so they return dicts (not coroutines)Both
authentication_error_payloadandmock_branch_create_errorare declared asasync defbut decorated with@pytest.fixture, so in a sync test they’ll return an un-awaited coroutine rather than the intended dict/HTTPXMock. To fix this, make them plain sync fixtures (or, if you truly need async fixtures, switch to@pytest_asyncio.fixtureand mark tests@pytest.mark.asyncio). Since your tests invoke the CLI synchronously, it’s simplest to convert them todef.• File
tests/unit/ctl/conftest.py
- Change
async def authentication_error_payload()→def authentication_error_payload()- Change
async def mock_branch_create_error(...)→def mock_branch_create_error(...)--- a/tests/unit/ctl/conftest.py +++ b/tests/unit/ctl/conftest.py @@ - @pytest.fixture - async def authentication_error_payload(): + @pytest.fixture + def authentication_error_payload(): response = { "data": None, "errors": [ {"message": "Authentication required", "extensions": {"code": "UNAUTHENTICATED"}}, ], } return response @@ - @pytest.fixture - async def mock_branch_create_error(httpx_mock: HTTPXMock) -> HTTPXMock: + @pytest.fixture + def mock_branch_create_error(httpx_mock: HTTPXMock) -> HTTPXMock: httpx_mock.add_response( status_code=400, method="POST", json={ "data": {"BranchCreate": None}, "errors": [{"message": "Branch already exists", "extensions": {"code": "BAD_USER_INPUT"}}], }, match_headers={"X-Infrahub-Tracker": "mutation-branch-create"}, ) return httpx_mockWith these changes,
authentication_error_payloadandmock_branch_create_errorwill return their payloads directly, andhttpx_mock.add_response(json=…)will receive a real dict rather than a coroutine.tests/unit/ctl/test_schema_app.py (1)
92-101: Align expected regex in tests with actual CLI outputThe mocked 422 payload uses Pydantic’s full pattern
^[A-Z][a-z0-9]+$, but the test’s assertion is still checking for the old/simplified pattern^[A-Z]+$. This will cause the test to fail because the CLI prints the exact pattern from the server’sctx["pattern"].Please update the test at tests/unit/ctl/test_schema_app.py to match the actual pattern (or loosen the check). For example:
• At line 119, change:
- assert "String should match pattern '^[A-Z]+$'" in output + assert "String should match pattern '^[A-Z][a-z0-9]+$'" in output• Similarly at line 123 (and any other occurrence), replace
^[A-Z]+$with^[A-Z][a-z0-9]+$.Alternatively, if you prefer a more resilient check, you could assert only on the key parts of the error without matching the entire regex, e.g.:
assert "type: string_pattern_mismatch" in output assert "input: \"OuT\"" in output assert "loc: [\"body\", \"schemas\", 0, \"nodes\", 0, \"namespace\"]" in outputtests/unit/sdk/test_utils.py (1)
107-131: Fix incorrect construction of a tuple in the TypeError test.
tuple("a", "b", "c")raises before callingstr_to_bool, so the error isn’t attributable to the function under test. Pass an actual tuple value to exercisestr_to_bool’s type check.Apply this diff:
- with pytest.raises(TypeError): - str_to_bool(tuple("a", "b", "c")) + with pytest.raises(TypeError): + str_to_bool(("a", "b", "c"))tests/unit/sdk/test_schema.py (1)
191-202: Wrong method called in sync branch of test_remove_enum_option_raises.The test name asserts removal should raise, but the sync path calls add_enum_option instead of remove_enum_option. This will mask regressions in the remove path for the sync client.
Apply this diff:
@@ async def test_remove_enum_option_raises(clients, client_type, mock_schema_query_01) -> None: if client_type == "standard": with pytest.raises(SchemaNotFoundError): await clients.standard.schema.remove_enum_option("DoesNotExist", "atribute", "option") with pytest.raises(ValueError): await clients.standard.schema.remove_enum_option("BuiltinTag", "attribute", "option") else: with pytest.raises(SchemaNotFoundError): - clients.sync.schema.add_enum_option("DoesNotExist", "atribute", "option") + clients.sync.schema.remove_enum_option("DoesNotExist", "atribute", "option") with pytest.raises(ValueError): - clients.sync.schema.add_enum_option("BuiltinTag", "attribute", "option") + clients.sync.schema.remove_enum_option("BuiltinTag", "attribute", "option")
🧹 Nitpick comments (57)
infrahub_sdk/pytest_plugin/items/graphql_query.py (1)
36-36: Nit: ensure URL renders cleanly as a stringExplicitly cast the URL to
strto avoid any repr-like formatting in some environments.- f"Failed {excinfo.value.request.method} on {excinfo.value.request.url}", + f"Failed {excinfo.value.request.method} on {str(excinfo.value.request.url)}",.vale/styles/Infrahub/sentence-case.yml (2)
41-43: Quote the ampersand entry to avoid YAML edge cases.Ampersand has special meaning in YAML (anchors). While valid here as a plain scalar, quoting prevents accidental parse issues during refactors and keeps linters quiet.
- - include_in_menu & menu_placement + - "include_in_menu & menu_placement"
66-69: Prefer token exceptions (“HTTP”, “HTTPS”) over a full-phrase exception.Adding the full heading “Separate proxies for HTTP and HTTPS” hard-codes a single variant. Token-level exceptions generalize better and reduce list bloat.
- - Separate proxies for HTTP and HTTPS + - HTTP + - HTTPSOptional: In a follow-up, relocate “HTTP/HTTPS” into the H section to preserve alphabetical ordering.
.github/workflows/repository-dispatch.yml (1)
45-45: Optional hardening: pin to a commit SHA.To reduce supply-chain risk, consider pinning
actions/checkoutto the v5 commit SHA (with a periodic Dependabot rule to refresh). Example:-uses: actions/checkout@v5 +uses: actions/checkout@<v5-commit-sha>.github/workflows/update-submodule.yml (1)
36-36: Optional hardening: pin to a commit SHA.Pin
actions/checkoutto the v5 commit SHA to guard against tag hijacking.-uses: actions/checkout@v5 +uses: actions/checkout@<v5-commit-sha>infrahub_sdk/testing/docker.py (1)
16-16: Docstring capitalization fix reads well; minor consistency nits nearby.Line 16 now capitalizes “Infrahub”. Consider aligning occurrences in the exception comment below for consistency.
Apply this diff within the same docstring block:
- # unreleased versions of infrahub, like `stable` or `develop` branch. - # For now, we consider this means we are testing against the most recent version of infrahub, + # unreleased versions of Infrahub, like `stable` or `develop` branch. + # For now, we consider this means we are testing against the most recent version of Infrahub,.github/workflows/labels.yml (1)
20-20: Optional: pin to SHA for supply-chain safety.-uses: actions/checkout@v5 +uses: actions/checkout@<v5-commit-sha>infrahub_sdk/schema/repository.py (1)
27-27: Minor grammar polish in docstring.“for typing purpose” → “for typing purposes.”
- """Class to regroup all elements of the Infrahub configuration for a repository for typing purpose.""" + """Class to regroup all elements of the Infrahub configuration for a repository for typing purposes.""".github/workflows/release.yml (1)
24-27: Pin checkout to a commit SHA and avoid persisting credentials.For supply-chain hardening and principle of least privilege, pin the action to its commit SHA and disable credential persistence since this job doesn’t push back to the repo.
Apply this diff:
- uses: "actions/checkout@v5" + uses: "actions/checkout@<commit-SHA-for-v5>" with: submodules: true + persist-credentials: falseIf you later need tags/history locally (e.g., changelog or versioning from git), also set
fetch-depth: 0on this step..github/workflows/sync-docs.yml (2)
18-22: For the source checkout, don’t persist credentials; also pin to a commit SHA.This step only reads from the source repo. Reduce token exposure and pin the action.
Apply this diff:
- - name: Checkout source repository - uses: actions/checkout@v5 + - name: Checkout source repository + uses: actions/checkout@<commit-SHA-for-v5> with: path: source-repo + persist-credentials: false
23-28: Pin target checkout to a commit SHA (keep credentials for pushing).You need credentials here for the later git push. Just pin the action to a SHA for supply-chain safety; keep credential persistence.
Apply this diff:
- - name: Checkout target repository - uses: actions/checkout@v5 + - name: Checkout target repository + uses: actions/checkout@<commit-SHA-for-v5> with: repository: opsmill/infrahub-docs token: ${{ secrets.PAT_TOKEN }} path: target-repo.github/workflows/publish-pypi.yml (1)
52-56: Pin checkout to a commit SHA and consider least-privilege defaults.This job doesn’t push to GitHub, so you can disable credential persistence. If the build uses git metadata for versioning, you may also need an unshallow fetch.
Apply this diff:
- - name: "Check out repository code" - uses: "actions/checkout@v5" + - name: "Check out repository code" + uses: "actions/checkout@<commit-SHA-for-v5>" with: submodules: true + persist-credentials: false + # fetch-depth: 0 # Uncomment if versioning/build relies on tags or full history.github/workflows/ci.yml (2)
137-147: Prefer npm ci over npm install in CI for reproducible docs buildsUsing the lockfile in CI reduces drift and speeds installs.
Apply:
- - name: "Install dependencies" - run: npm install + - name: "Install dependencies" + run: npm ci
49-49: Allactions/checkoutusages are pinned to v5—no stragglers found; recommend pinning other third-party actions to commit SHAs for stronger supply-chain security
Verified that every
actions/checkoutreference across all workflows has been updated to@v5—there are no v3/v4 remnants.However, several other actions are still using mutable semantic-version tags. To harden your CI pipelines, pin each to a specific commit SHA (e.g.
uses: peter-evans/repository-dispatch@<full-commit-sha> # rather than @v3). Below is a non-exhaustive list of candidates:
• .github/workflows/repository-dispatch.yml:
– uses: peter-evans/repository-dispatch@v3
• .github/workflows/ci.yml:
– uses: dorny/paths-filter@v3
– uses: DavidAnson/markdownlint-cli2-action@v20
– uses: actions/setup-node@v4
– uses: actions/setup-python@v5
• .github/workflows/labels.yml:
– uses: crazy-max/[email protected]
• .github/workflows/publish-pypi.yml:
– uses: snok/install-poetry@v1
– uses: actions/cache@v4
• .github/workflows/release.yml:
– uses: snok/install-poetry@v1You can retrieve each action’s latest commit SHA via GitHub’s API or
git ls-remote, then replace the tag with that SHA.No further non-v5
actions/checkoutoccurrences were detected—your sweep is complete.infrahub_sdk/utils.py (1)
98-98: JsonDecodeError.url cast to str — good; consider truncating logged contentCasting the URL avoids type surprises in error handling. Minor improvement: cap response.text included in exceptions to avoid leaking large payloads or secrets into logs.
Apply:
def decode_json(response: httpx.Response) -> dict: try: return response.json() except json.decoder.JSONDecodeError as exc: - raise JsonDecodeError(content=response.text, url=str(response.url)) from exc + # Keep error context without dumping potentially large/sensitive bodies + content_snippet = response.text[:2048] + raise JsonDecodeError(content=content_snippet, url=str(response.url)) from exctests/unit/sdk/test_diff_summary.py (2)
94-94: Name test parameters for clearer ids in pytest output.Adding ids makes it easier to spot which client failed when this test breaks.
-@pytest.mark.parametrize("client_type", client_types) +@pytest.mark.parametrize("client_type", client_types, ids=client_types)
107-160: Make membership assertions resilient to additional fields.
assert {…} in node_diffsrequires exact dict equality. If the SDK adds fields, these assertions will fail even when the relevant subset matches. Prefer subset checks.- assert { + expected = { "branch": "branch2", "kind": "TestCar", "id": "17fbadf0-6637-4fa2-43e6-1677ea170e0f", "action": "None", "display_label": "nolt #444444", "elements": [ { "action": "UPDATED", "element_type": "RELATIONSHIP_ONE", "name": "owner", "summary": {"added": 0, "removed": 0, "updated": 1}, } ], - } in node_diffs + } + assert any(expected.items() <= nd.items() for nd in node_diffs) @@ - assert { + expected = { "branch": "branch2", "kind": "TestPerson", "id": "17fbadf0-634f-05a8-43e4-1677e744d4c0", "action": "None", "display_label": "Jane", "elements": [ { "action": "UPDATED", "element_type": "RELATIONSHIP_MANY", "name": "cars", "summary": {"added": 0, "removed": 1, "updated": 0}, "peers": [{"action": "REMOVED", "summary": {"added": 0, "removed": 3, "updated": 0}}], } ], - } in node_diffs + } + assert any(expected.items() <= nd.items() for nd in node_diffs) @@ - assert { + expected = { "branch": "branch2", "kind": "TestPerson", "id": "17fbadf0-6243-5d3c-43ee-167718ff8dac", "action": "None", "display_label": "Jonathan", "elements": [ { "action": "UPDATED", "element_type": "ATTRIBUTE", "name": "name", "summary": {"added": 0, "removed": 0, "updated": 1}, }, { "action": "UPDATED", "element_type": "RELATIONSHIP_MANY", "name": "cars", "summary": {"added": 1, "removed": 0, "updated": 0}, "peers": [{"action": "ADDED", "summary": {"added": 3, "removed": 0, "updated": 0}}], }, ], - } in node_diffs + } + assert any(expected.items() <= nd.items() for nd in node_diffs)tests/integration/test_schema.py (1)
19-22: Avoid brittle string equality on exception messages.Exact message matches tend to break with benign wording changes. Use pytest’s
matchto assert the important parts.- with pytest.raises(BranchNotFoundError) as exc: - await client.all(kind="BuiltinTag", branch="I-do-not-exist") - - assert str(exc.value) == "The requested branch was not found on the server [I-do-not-exist]" + with pytest.raises( + BranchNotFoundError, match=r"branch was not found.*\[I-do-not-exist\]" + ): + await client.all(kind="BuiltinTag", branch="I-do-not-exist")tests/integration/test_repository.py (1)
17-20: Introduce public API for initial commit and remove internal couplingVerified that
infrahub_sdk/testing/repository.py’sGitRepoclass does not expose any public methods or constants for retrieving the head commit or its message, and the initial commit message is hard-coded in the implementation. To decouple tests from internal implementation details and avoid fragile failures when the default message or commit behavior changes, please consider:• Adding a class-level constant for the default message in
infrahub_sdk/testing/repository.py:@dataclass class GitRepo: # … INITIAL_COMMIT_MESSAGE: str = "First commit" # …• Defining a public method to retrieve the latest commit:
def head_commit(self): """Return the current HEAD commit (dulwich.objects.Commit).""" return self._repo.git[self._repo.git.head()]• Defining a public property for the commit count:
@property def commit_count(self) -> int: """Return the number of commits in the repository.""" return len(list(self._repo.git.get_walker()))• Updating
tests/integration/test_repository.py(Lines 17–20) to use the new API:- commit = repo._repo.git[repo._repo.git.head()] - assert len(list(repo._repo.git.get_walker())) == 1 - assert commit.message.decode("utf-8") == "First commit" + commit = repo.head_commit() + assert repo.commit_count == 1 + assert commit.message.decode("utf-8") == GitRepo.INITIAL_COMMIT_MESSAGEThis change centralizes the initial-commit contract in one location, prevents tests from reaching into private attributes, and makes future updates (e.g., changing the default message) straightforward and safe.
tests/unit/sdk/test_protocols_generator.py (2)
40-41: Remove duplicate assertion.The two asserts are identical and add no value. Keep one.
- assert CodeGenerator._jinja2_filter_syncify(value=test_case.input, sync=test_case.sync) == test_case.output - assert CodeGenerator._jinja2_filter_syncify(value=test_case.input, sync=test_case.sync) == test_case.output + assert CodeGenerator._jinja2_filter_syncify(value=test_case.input, sync=test_case.sync) == test_case.output
39-41: Avoid testing private method directly.
_jinja2_filter_syncifyis an internal helper. Prefer exercising it via the public rendering path (or expose a stable helper) to reduce test brittleness when internals change.tests/unit/sdk/checks/test_checks.py (1)
37-43: Use the injected client fixture (or drop it) to avoid hidden coupling to defaultsMinor: This test currently doesn’t use the provided client fixture, relying on IFCheck.init() to create its own client with default configuration. To reduce hidden coupling to defaults and make the test more explicit, pass the fixture into init and assert identity.
Apply this diff:
- check = await IFCheck.init() - assert isinstance(check.client, InfrahubClient) + check = await IFCheck.init(client=client) + assert check.client is clienttests/integration/test_infrahubctl.py (1)
45-46: Fixture annotated with -> None: good clarityExplicitly annotating base_dataset as returning None is consistent with the broader typing pass and with how the fixture is used (presence-only side effects).
Optionally, refine the repository fixture type for stricter typing (outside this hunk):
from collections.abc import Generator @pytest.fixture(scope="class") def repository() -> Generator[str, None, None]: ...tests/unit/ctl/test_branch_app.py (1)
27-32: LGTM; minor robustness note on message matchingThe substring assertion is pragmatic. If CLI output ever gains ANSI color, consider stripping color as done in validate_app tests to keep this assertion resilient.
tests/unit/ctl/test_validate_app.py (1)
36-41: xfail FIXME: offer to help make this passWhen you’re ready to address the FIXME, I can help trace which exception is thrown for invalid JSON and adjust the CLI error handling and test accordingly. Happy to open an issue with concrete reproduction steps.
tests/unit/sdk/test_object_store.py (2)
47-54: Annotate parameter type for clarity (method: str)Parametrize supplies strings; adding a type makes intent explicit and improves editor feedback.
-async def test_validate_method_signature(method) -> None: +async def test_validate_method_signature(method: str) -> None:
57-66: Type the test parameters for consistency with other modulesOther tests (e.g., test_batch.py) annotate BothClients and client_type. Mirroring that here aids readability and static checks.
-async def test_object_store_get(client_type, clients, mock_get_object_store_01) -> None: +async def test_object_store_get(client_type: str, clients: BothClients, mock_get_object_store_01: HTTPXMock) -> None: @@ -async def test_object_store_upload(client_type, clients, mock_upload_object_store_01) -> None: +async def test_object_store_upload(client_type: str, clients: BothClients, mock_upload_object_store_01: HTTPXMock) -> None:Add missing typing import (top-level), reusing existing HTTPXMock import:
from typing import TYPE_CHECKING if TYPE_CHECKING: from tests.unit.sdk.conftest import BothClientsAlso applies to: 69-77
tests/unit/sdk/test_store.py (3)
11-20: Add parameter type hints (client_type, clients) to match suite-wide styleKeeps tests uniform and helps tools.
-def test_node_store_set(client_type, clients, schema_with_hfid) -> None: +def test_node_store_set(client_type: str, clients: BothClients, schema_with_hfid) -> None: @@ -def test_node_store_set_no_hfid(client_type, clients, location_schema) -> None: +def test_node_store_set_no_hfid(client_type: str, clients: BothClients, location_schema) -> None:Supplemental import:
from typing import TYPE_CHECKING if TYPE_CHECKING: from tests.unit.sdk.conftest import BothClientsAlso applies to: 36-45
70-79: Add parameter type hints for get() testsMirror the same typing as above for consistency.
-def test_node_store_get(client_type, clients, location_schema) -> None: +def test_node_store_get(client_type: str, clients: BothClients, location_schema) -> None: @@ -def test_node_store_get_with_hfid(client_type, clients, schema_with_hfid) -> None: +def test_node_store_get_with_hfid(client_type: str, clients: BothClients, schema_with_hfid) -> None:Also applies to: 114-123
11-67: Consider avoiding assertions on private attributes to reduce brittlenessThese tests assert on internals like _branches/_objs/_keys (e.g., Lines 30-33, 55-59, 63-66). Prefer validating via public APIs (store.get(...), returned node ids/HFIDs) where feasible; it decouples tests from internal structure changes. If internals must be inspected, confine to a dedicated “internals” test.
tests/unit/sdk/test_branch.py (2)
24-31: Type the parametrized identifier (method: str)Small clarity win; consistent with similar signature checks elsewhere.
-def test_validate_method_signature(method) -> None: +def test_validate_method_signature(method: str) -> None:
34-42: Type the fixtures/params for parity with other testsAnnotate clients and client_type similarly to tests in test_batch.py.
-async def test_get_branches(clients, mock_branches_list_query, client_type) -> None: +async def test_get_branches(clients: BothClients, mock_branches_list_query, client_type: str) -> None:Supplemental import:
from typing import TYPE_CHECKING if TYPE_CHECKING: from tests.unit.sdk.conftest import BothClientstests/unit/sdk/test_repository.py (1)
62-68: Reduce fixture-coupling on commit message assertionAsserting the exact message ties this test to fixture content. Prefer a looser check so fixture wording can evolve without breaking the test.
- assert commit.message.decode("utf-8") == "First commit" + assert "First commit" in commit.message.decode("utf-8")Optional: expose a public accessor on GitRepo for the head commit to avoid reaching into repo._repo from tests.
docs/docs/python-sdk/guides/client.mdx (2)
95-101: Tighten wording: “Export the variables” (plural) and use imperative styleThese sentences currently use “We need to export … as an environment variable” while exporting multiple variables. Suggest rephrasing to concise imperative style.
- We need to export the address and API token as an environment variable: + Export the address and API token as environment variables:Apply similarly to the username/password sections.
Also applies to: 124-134, 160-171, 190-201
74-74: Use extensionless internal links to match Docusaurus routingTo keep links consistent with the “Related resources” section:
-You can find the full list of configuration options in the [SDK configuration reference](../reference/config.mdx). +You can find the full list of configuration options in the [SDK configuration reference](../reference/config).tests/unit/sdk/test_group_context.py (2)
10-11: Remove unused variable to keep linters quiet
client_types = ["standard", "sync"]is defined but unused. Dropping it reduces noise for dead-code checks.-client_types = ["standard", "sync"] -
59-66: Avoid brittle assertions against a hard-coded hash suffixAsserting the entire generated group name including a fixed hash makes the test fragile if the hashing/serialization logic evolves. Prefer structural assertions.
Example refactor:
-assert context._generate_group_name() == "MYID-11aaec5206c3dca37cbbcaaabf121550" +import re +name = context._generate_group_name() +assert name.startswith("MYID-") +assert re.fullmatch(r"[a-f0-9]{32}", name.split("-", 1)[1]) is not NoneRepeat similarly for the
suffix="xxx"case by checking prefixMYID-xxx-and a 32-hex tail.CLAUDE.md (5)
149-155: Add npm install/ci step before starting the docs dev serverNew contributors will hit module-not-found errors without installing docs dependencies first. Suggest adding npm ci explicitly.
Apply this diff:
# Start development server (requires Node.js) -cd docs && npm start +cd docs +npm ci +npm start
91-104: Tighten wording for the dual client pattern sectionMinor phrasing tweaks improve clarity and style.
-All operations follow dual implementation pattern: +All operations follow a dual‑implementation pattern:
107-110: Clarify proxy configuration precedence and exclusivityThe note mentions “mutual exclusivity” but doesn’t state behavior when both are set. Explicitly documenting that we raise a validation error (and which variable takes precedence if we ever relax this) will reduce confusion.
Would you like me to propose a short paragraph here that states: “If both INFRAHUB_PROXY and INFRAHUB_PROXY_MOUNTS_HTTP/HTTPS are set, configuration validation fails with a clear error. No precedence is applied.”?
161-166: Avoid pinning tool versions in prose“Ruff (0.11.0)” will age quickly. Prefer omitting the version or indicating “>=” to keep the doc evergreen; the exact version already lives in pyproject.toml.
-- **Ruff**: Comprehensive linting and formatting (0.11.0) +- **Ruff**: Comprehensive linting and formatting
72-76: Minor style: compound adjectives and list formattingConsider “Lazy loading” and “Batch operations” in sentence case, and remove dash carry‑over to avoid mid‑sentence punctuation glitches.
-- **Lazy Loading**: Nodes load attributes and relationships on demand -- **Batch Operations**: Support for bulk create/update/delete operations +- **Lazy loading**: Nodes load attributes and relationships on demand +- **Batch operations**: Support for bulk create/update/delete operationstests/unit/sdk/test_task.py (1)
105-156: Optional: reduce duplication in standard/sync branchesSeveral tests branch on client_type to call async vs. sync methods. A small helper (factory returning the appropriate task manager or a wrapper that awaits conditionally) can cut repetition and sharpen intent.
If you want, I can draft a helper like:
def get_task_api(clients, client_type): return clients.standard.task if client_type == "standard" else clients.sync.taskand update calls accordingly.
tests/unit/ctl/test_cli.py (1)
29-33: Consider stabilizing assertions against structured outputString-contains assertions on human-readable CLI output tend to be brittle. If the command supports a machine-readable flag (e.g., --json), assert on parsed fields instead.
If --json doesn’t exist yet, I can open a follow-up to add it to info/info --detail and update tests accordingly.
tests/unit/sdk/test_timestamp.py (2)
66-69: Optional: add parameter type annotations for stronger checksAnnotating parameters improves mypy coverage and self-documentation.
-def test_to_datetime(input_str, expected_datetime) -> None: +def test_to_datetime(input_str: str, expected_datetime: datetime) -> None: @@ -def test_to_string_default(input_str, expected_str, expected_str_no_z) -> None: +def test_to_string_default(input_str: str, expected_str: str, expected_str_no_z: str) -> None: @@ -def test_invalid_raises_correct_error(invalid_str) -> None: +def test_invalid_raises_correct_error(invalid_str: str) -> None:Also applies to: 88-92, 132-135
30-43: Minor: relying on a private method in testsUsing Timestamp._parse_string in tests is acceptable but couples tests to internals. If coverage of the public API suffices, prefer parsing only via the constructor and public methods.
Happy to draft equivalent tests that exercise parsing through Timestamp(...) and to_string()/to_datetime() exclusively.
tests/unit/sdk/test_store_branch.py (1)
20-23: Reduce coupling to private attributes in assertionsThese tests assert directly on _objs/_keys. While fine for white-box tests, adding at least one assertion via the public API will make failures more actionable if internals change.
assert node._internal_id in store._objs assert "mykey" in store._keys assert store._keys["mykey"] == node._internal_id + # Also validate via the public API: + assert store.get(key="mykey").id == getattr(node, "id", store.get(key=node._internal_id).id)I can propagate a similar public-API assertion to the other set* test for consistency.
tests/integration/test_spec_object.py (2)
56-57: Strengthen the assertion after creating a branch.Capture the returned branch and assert basic invariants (id present). This mirrors patterns elsewhere in the suite and guards against silent API regressions.
Apply this diff:
- await client.branch.create(branch_name=branch_name, sync_with_git=False) + branch = await client.branch.create(branch_name=branch_name, sync_with_git=False) + assert getattr(branch, "id", None) is not None
138-139: Minor readability nit: avoid reusing the name “menu” as both iterable and loop variable.While correct, the dict comprehension is harder to read because it shadows the outer variable. Renaming the loop variable improves clarity.
Apply this diff:
- menu = await client.filters(kind=menu_file.spec.kind, protected__value=False, branch=branch_name) - assert len(menu) == 3 - - menu_by_name = {menu.display_label: menu for menu in menu} + menus = await client.filters(kind=menu_file.spec.kind, protected__value=False, branch=branch_name) + assert len(menus) == 3 + + menu_by_name = {m.display_label: m for m in menus}tests/unit/sdk/test_utils.py (2)
198-213: Use TemporaryDirectory as a context manager to ensure cleanup on failure.Manual
.cleanup()may be skipped if an assertion fails before reaching it. The context manager pattern is safer and simpler.Apply this diff:
- tmp_dir = tempfile.TemporaryDirectory() - directory = Path(tmp_dir.name) + with tempfile.TemporaryDirectory() as tmp_dir: + directory = Path(tmp_dir) - with pytest.raises(FileExistsError): - write_to_file(directory, "placeholder") + with pytest.raises(FileExistsError): + write_to_file(directory, "placeholder") - assert write_to_file(directory / "file.txt", "placeholder") is True - assert write_to_file(directory / "file.txt", "placeholder") is True - assert write_to_file(directory / "file.txt", "") is True - assert write_to_file(directory / "file.txt", None) is True - assert write_to_file(directory / "file.txt", 1234) is True - assert write_to_file(directory / "file.txt", {"key": "value"}) is True - - tmp_dir.cleanup() + assert write_to_file(directory / "file.txt", "placeholder") is True + assert write_to_file(directory / "file.txt", "placeholder") is True + assert write_to_file(directory / "file.txt", "") is True + assert write_to_file(directory / "file.txt", None) is True + assert write_to_file(directory / "file.txt", 1234) is True + assert write_to_file(directory / "file.txt", {"key": "value"}) is True
215-230: Prevent flakiness intest_calculate_time_diffThe current test relies on calling Instant.now() twice (once to build the test timestamp and once inside calculate_time_diff), so any delay between those calls—especially on slower CI agents—can push the nanosecond rounding past 0.5 s and increment the seconds by 1. To keep the assertions deterministic, either freeze “now” in the test or allow a ±1 s tolerance.
Affected location
• tests/unit/sdk/test_utils.py lines 215–230Suggested approaches:
• Option A (tolerant assertions)
Allow the result to be either the exact expected string or one second higher:time1 = Instant.now().subtract(seconds=98).format_common_iso() - assert calculate_time_diff(time1) == "1m and 38s ago" + assert calculate_time_diff(time1) in {"1m and 38s ago", "1m and 39s ago"} time2 = Instant.now().subtract(hours=1, minutes=12, seconds=34).format_common_iso() - assert calculate_time_diff(time2) == "1h 12m and 34s ago" + assert calculate_time_diff(time2) in {"1h 12m and 34s ago", "1h 12m and 35s ago"}• Option B (freeze time)
Wrap the test in a deterministic “now” using freezegun or by monkey-patching your Timestamp/Instant factory. For example, with freezegun:from freezegun import freeze_time @freeze_time("2025-01-01T00:00:00Z") def test_calculate_time_diff(): # Now Instant.now() and Timestamp() both yield 2025-01-01T00:00:00Z time1 = Instant.now().subtract(seconds=98).format_common_iso() assert calculate_time_diff(time1) == "1m and 38s ago" …To verify locally, you can still repeat the test 50× to ensure no intermittent failures:
for i in {1..50}; do pytest -q tests/unit/sdk/test_utils.py::test_calculate_time_diff || exit 1 donetests/unit/sdk/test_query_analyzer.py (1)
154-175: Optional: add parameter type hints in the parametrized test.Annotating
var_type: str, var_required: boolclarifies expectations and aids editors.Apply this diff:
-async def test_get_nested_variables(var_type, var_required) -> None: +async def test_get_nested_variables(var_type: str, var_required: bool) -> None:tests/unit/sdk/test_artifact.py (1)
69-71: LGTM: artifact fetch test annotated; content assertion kept.If desired, consider normalizing trailing newline to avoid brittle text comparisons, but current form is acceptable.
tests/integration/test_infrahub_client.py (2)
784-799: Minor: sync-branch baseline captured from standard client in test_clone_define_branch.In the sync branch, original_branch is pulled from clients.standard instead of clients.sync. Likely harmless if branches are equal, but it’s inconsistent.
@@ async def test_clone_define_branch(clients: BothClients, client_type: str) -> None: - else: - original_branch = clients.standard.default_branch + else: + original_branch = clients.sync.default_branch clone = clients.sync.clone(branch="my_other_branch") assert clients.sync.store._default_branch == original_branch assert isinstance(clone, InfrahubClientSync) assert clients.sync.default_branch != clone.default_branch
35-41: Optional: add return type to set_pagination_size3 fixture for typing consistency.Small consistency nit: other fixtures gained -> None in this PR. You can mirror that here.
@pytest.fixture -async def set_pagination_size3(self, client: InfrahubClient): +async def set_pagination_size3(self, client: InfrahubClient) -> None: original_pagination_size = client.pagination_size client.pagination_size = 3 yield client.pagination_size = original_pagination_sizetests/unit/sdk/test_client.py (1)
85-152: Reduce duplication across standard/sync client branches in tests.Many tests branch on client_type to call async vs sync methods, duplicating assertions. Consider a small helper or parametrized adapter to normalize calls and cut repetition.
Example pattern:
def run_client(op_async, op_sync, client_type, *args, **kwargs): return (op_async(*args, **kwargs) if client_type == "standard" else op_sync(*args, **kwargs)) # Usage in tests (async context): result = await run_client(clients.standard.all, clients.sync.all, client_type, kind="CoreRepository")This keeps assertions single-sourced and reduces maintenance.
Also applies to: 154-172, 174-212, 224-271, 321-366, 403-487, 512-611, 736-766
tests/unit/sdk/test_node.py (1)
1782-1784: Minor: inconsistent class used in sync branch.In test_update_input_data_empty_relationship, the sync branch initializes tag_blue with InfrahubNode instead of InfrahubNodeSync. It likely still works due to shared interface, but it’s inconsistent.
- tag_blue = InfrahubNode(client=client, schema=tag_schema, data=tag_blue_data) + tag_blue = InfrahubNodeSync(client=client, schema=tag_schema, data=tag_blue_data)
| ```python | ||
| from infrahub_sdk import InfrahubClient, Config | ||
| config = Config(address="http://localhost:8000", api_token="token", default_branch="other-branch") | ||
| client_other_branch = InfrahubClient(config=config) | ||
|
|
||
| tag_other_branch = await client_other_branch.get(kind="BuiltinTag", name__value="RED") | ||
| tag_main_branch = await client_other_branch.get(kind="BuiltinTag", name__value="RED", branch="main") | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix top-level await in Step 3 code snippets (invalid Python) and show both Async/Sync variants
The Step 3 “Code” and “Environment” blocks use await at top-level, which will raise a SyntaxError in Python. Wrap calls in an async entrypoint (and asyncio.run) for the async client, and provide a Sync variant without await. Consider mirroring the Tabs pattern used in Step 1/2.
Apply this minimal fix for the Async snippet:
-```python
-from infrahub_sdk import InfrahubClient, Config
-config = Config(address="http://localhost:8000", api_token="token", default_branch="other-branch")
-client_other_branch = InfrahubClient(config=config)
-
-tag_other_branch = await client_other_branch.get(kind="BuiltinTag", name__value="RED")
-tag_main_branch = await client_other_branch.get(kind="BuiltinTag", name__value="RED", branch="main")
-```
+```python
+import asyncio
+from infrahub_sdk import InfrahubClient, Config
+config = Config(address="http://localhost:8000", api_token="token", default_branch="other-branch")
+
+async def main():
+ client_other_branch = InfrahubClient(config=config)
+ tag_other_branch = await client_other_branch.get(kind="BuiltinTag", name__value="RED")
+ tag_main_branch = await client_other_branch.get(kind="BuiltinTag", name__value="RED", branch="main")
+
+asyncio.run(main())
+```For Sync, show:
from infrahub_sdk import InfrahubClientSync, Config
config = Config(address="http://localhost:8000", api_token="token", default_branch="other-branch")
client_other_branch = InfrahubClientSync(config=config)
tag_other_branch = client_other_branch.get(kind="BuiltinTag", name__value="RED")
tag_main_branch = client_other_branch.get(kind="BuiltinTag", name__value="RED", branch="main")If you’d like, I can provide a full patch that nests Step 3 under Async/Sync tabs for both “Code” and “Environment.”
Also applies to: 237-243
| export INFRAHUB_API_TOKEN="06438eb2-8019-4776-878c-0941b1f1d1ec" # This is the default token | ||
| ``` |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid publishing a concrete API token value in docs
Even if this is a non-sensitive default token, readers may copy/paste it into real scripts. Use a placeholder and clarify where to get a real token.
-export INFRAHUB_API_TOKEN="06438eb2-8019-4776-878c-0941b1f1d1ec" # This is the default token
+export INFRAHUB_API_TOKEN="<your-api-token>"Optionally add a note pointing to how to create tokens in Infrahub.
To confirm whether this exact token appears in code/tests (and is safe), run:
🏁 Script executed:
#!/bin/bash
rg -n -C2 -P '06438eb2-8019-4776-878c-0941b1f1d1ec|INFRAHUB_API_TOKEN' .Length of output: 3887
Replace hard-coded API token with a placeholder in documentation
We’ve verified that the literal token 06438eb2-8019-4776-878c-0941b1f1d1ec only appears in docs (no code or tests reference it), so readers copying examples could inadvertently use the wrong token. To avoid confusion and promote best practices:
- In
docs/docs/python-sdk/guides/client.mdx(lines 319–320), update the snippet to:-export INFRAHUB_API_TOKEN="06438eb2-8019-4776-878c-0941b1f1d1ec" # This is the default token +export INFRAHUB_API_TOKEN="<your-api-token>"
- Add a brief note above or below explaining how users can generate or retrieve their own API tokens from the Infrahub dashboard (e.g. “Obtain an API token at https://app.infrahub.io/account/tokens”).
Optional follow-up for consistency:
- In
docs/docs/infrahubctl/infrahubctl.mdx(lines 32 and 41), replace the hard-coded token in both the table andinfrahubctl.tomlexample with a placeholder (e.g.<your-api-token>).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export INFRAHUB_API_TOKEN="06438eb2-8019-4776-878c-0941b1f1d1ec" # This is the default token | |
| ``` | |
| export INFRAHUB_API_TOKEN="<your-api-token>" |
🤖 Prompt for AI Agents
In docs/docs/python-sdk/guides/client.mdx around lines 319-320, replace the
hard-coded API token string with a generic placeholder (e.g. <your-api-token> or
YOUR_API_TOKEN) and add a brief one-line note immediately above or below the
snippet pointing users to obtain their token from the Infrahub dashboard (e.g.
"Obtain an API token at https://app.infrahub.io/account/tokens"); optionally,
for consistency also update docs/docs/infrahubctl/infrahubctl.mdx at the
referenced lines (32 and 41) to use the same placeholder in the table and
infrahubctl.toml example.
3 participants
Summary by CodeRabbit