Initial support for file upload#71
Conversation
Deploying nornir-infrahub with
|
| Latest commit: |
9de8e3b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f9a351b0.nornir-infrahub.pages.dev |
| Branch Preview URL: | https://001-object-file-support.nornir-infrahub.pages.dev |
a9bccf9 to
e155a78
Compare
|
We should not use the term |
| ) | ||
|
|
||
|
|
||
| def _get_client(task: Task) -> InfrahubClientSync: |
There was a problem hiding this comment.
We should probably move this a utils module. This could be useful in other contexts?
There was a problem hiding this comment.
Moved to nornir_infrahub/utils.py as get_client(task) and imported from file_object.py. Good for the artifact.py tasks to adopt too; left that for a follow-up to keep this PR focused. (commit b35f894)
|
The SDK provides a capability to create a FileObject from a bytes object as well. We should probably add support for this https://docs.infrahub.app/python-sdk/sdk_ref/infrahub_sdk/node#upload_from_bytes |
| data = data or {} | ||
|
|
||
| # Look up existing node | ||
| existing_node = None |
There was a problem hiding this comment.
Do we need to implement this here? AFAIK the SDK behaves in a idempotent way already when using allow_upsert=True. Ofcourse this requires that you have setup a proper unique constraint and/or HFID
There was a problem hiding this comment.
allow_upsert=True avoids the duplicate-create, but it still re-uploads the file bytes on every call and doesn't let us report changed=False to Nornir — the whole point of the client-side SHA-1 compare is (a) skip the upload entirely when the content hasn't changed, and (b) drive Nornir's changed flag correctly (matches how nornir_utils.write_file behaves). I'd like to keep the checksum-skip for those reasons. That said, allow_upsert is now reachable via the new **kwargs passthrough, so callers who do want upsert semantics on create can opt in.
| data: dict[str, Any] | None = None, | ||
| node_id: str | None = None, | ||
| hfid: list[str] | None = None, | ||
| branch: str | None = None, |
There was a problem hiding this comment.
Should we support kwargs here as well, to pass arguments to client.create
There was a problem hiding this comment.
Added **kwargs passthrough on upload_file_object. It forwards to client.create(...), so callers can pass allow_upsert, timeout, request_context, etc. New test test_upload_forwards_kwargs_to_create covers it.
| def upload_file_object( | ||
| task: Task, | ||
| kind: str, | ||
| file_path: str, |
There was a problem hiding this comment.
Done. file_path now accepts str | Path and we normalise to Path internally.
| kind: str, | ||
| node_id: str | None = None, | ||
| hfid: list[str] | None = None, | ||
| dest: str | None = None, |
There was a problem hiding this comment.
Use Path here? Also we could come up with a better name for this argument that is a little bit more descriptive?
Also we can probably make this behave in a idempotent fashion? IE the task result should report only if a change was made when a file really changed? Similar to how the write_file in nornir_utils behaves?
There was a problem hiding this comment.
All three addressed:
- Path:
save_toacceptsstr | Path. - Rename:
dest→save_to(more descriptive — "where to save the file locally"). - Idempotent:
download_file_objectnow compares the local file's SHA-1 to the server checksum before downloading. If they match the write is skipped andchanged=False; otherwise we download, overwrite, andchanged=True. Same model asnornir_utils.plugins.tasks.files.write_file. Covered by new teststest_download_skips_write_when_local_matchesandtest_download_overwrites_when_local_differs.
|
@wvandeun — review feedback addressed in b35f894. Summary: Top-level comments
Inline comments — replied on each thread; in short:
Tests: 38/38 pass. Ruff, ruff format, ty, pylint all clean. |
There was a problem hiding this comment.
1 issue found across 19 files
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
|
|
||
| ```python | ||
| from nornir import InitNornir | ||
| from nornir.core.plugins.inventory import InventoryPluginRegister |
There was a problem hiding this comment.
these 2 imports should not be required
| from nornir_infrahub.plugins.inventory.infrahub import InfrahubInventory | ||
|
|
||
| def main(): | ||
| # Register the custom InfrahubInventory plugin |
|
If we need the idempotent behavior for both uploading and downloading, then it should not really be part of the nornir plugin, but it should be provided by the SDK itself. If not we are going to have to re-implement this in all integrations. |
585dcf2 to
982962f
Compare
…ical .markdownlint.yml PR #71 originally added a root .markdownlint.json, but the repo's canonical config is .markdownlint.yml (docusaurus-aware: MD033/MD007/MD029/MD047 etc.). markdownlint-cli prefers .json over .yml, so our file shadowed the canonical one and #55's guide/topic docs failed markdown-lint after the rebase. Remove the .json and add the two rules the generated plugin reference tables need (MD012 multiple-blanks, MD060 table padding) to .markdownlint.yml. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ical .markdownlint.yml PR #71 originally added a root .markdownlint.json, but the repo's canonical config is .markdownlint.yml (docusaurus-aware: MD033/MD007/MD029/MD047 etc.). markdownlint-cli prefers .json over .yml, so our file shadowed the canonical one and #55's guide/topic docs failed markdown-lint after the rebase. Remove the .json and add the two rules the generated plugin reference tables need (MD012 multiple-blanks, MD060 table padding) to .markdownlint.yml. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1043710 to
8c45a30
Compare
There was a problem hiding this comment.
5 issues found across 16 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Testing statusThis refactor (moving the Integration coverage is intentionally deferred until #79 (the testcontainers-based integration harness) merges to Holding off on merge until that integration pass is done. |
minitriga
left a comment
There was a problem hiding this comment.
Code Review
Overview
Adds two new Nornir tasks (upload_file_object, download_file_object) for Infrahub CoreFileObject nodes, mirroring opsmill/infrahub-ansible#317. Includes a shared get_client() helper, 28 unit tests, doc-template fixes, and an SDK bump. Overall well-built: idempotency semantics are correct, error handling follows the Nornir Result(failed=True) pattern, and test coverage is thorough. SDK interaction details verified below.
Verified correctness ✅
- SDK methods exist and behave as assumed.
upload_if_changed()/matches_local_checksum()are absent in 1.19.0 and present in 1.20.1 (checked both), so the bump to>=1.20.1is genuinely required. - Attrs + file both changed is safe. Attributes are staged via
setattrbeforeupload_if_changed(), and the SDK'supload_if_changed()callsself.save()on upload — staged attrs persist. The explicitsave()on the skip path covers the other branch. Tests cover both (TestUploadAttrsOnSkip). - Download checksum guard is correct.
matches_local_checksum()raisesValueErrorwhen there's no server checksum, but the code guards withserver_checksum and ...first.
Issues
1. Relationships in data are silently dropped on update (medium)
The update path in file_object.py only applies keys found in existing_obj._schema.attribute_names:
if attr_name in existing_obj._schema.attribute_names:On create, relationships in data flow through client.create(data=data) fine. On update, they're silently ignored. The PR's own usage example passes data={"artifact": artifact_id} — presumably a relationship — so the documented example works on first run and silently no-ops on subsequent runs. Either handle relationship_names too, or document that data only updates attributes on existing objects (and ideally warn/fail on unknown keys instead of skipping).
2. Nonexistent explicit object_id falls through to create (medium)
_lookup_existing_object returns None on NodeNotFoundError regardless of how the object was identified. If a user passes a typo'd UUID, the task creates a new node instead of failing. Upsert-on-miss is defensible for hfid or the file_name fallback, but an explicit UUID that doesn't exist is almost certainly an error — consider failing there.
3. PR description is stale (low)
- Says bump to
>=1.19.0; the diff bumps to>=1.20.1(correctly). - Names
upload_from_path(),download_file(),is_file_object()as the motivating methods; the code actually depends onupload_if_changed()andmatches_local_checksum()(1.20.x-only).
4. Global ty rule disables are too blunt (low)
pyproject.toml adds project-wide invalid-return-type = "ignore" and invalid-assignment = "ignore", while file_object.py also carries inline # type: ignore[return-type] / [assignment] comments for the same spots. The inline ignores make the global disables redundant — and the global ones silence those rules for all future code. Prefer keeping only the inline ignores.
5. Minor nits
_lookup_existing_objectonly catchesNodeNotFoundError; thefile_name__valuefallback will raise an unhandledIndexErrorfrom the SDK if multiple nodes share a file name (Nornir still marks the task failed, but with a raw traceback rather than a clean message).str(save_to).endswith("/")won't detect a trailing\on Windows —save_to_path.is_dir()covers existing dirs, so this only affects not-yet-created directory paths.download_file_objectholds full content in memory and embeds base64 in everyResult— Nornir aggregates results across hosts, so this can get heavy for large binaries across many hosts. Maybe worth a docstring note.**kwargsis forwarded only on the create path, not update — documented that way, just noting the asymmetry (e.g.timeoutwon't apply to updates).
Code quality & conventions 👍
utils.get_client()matches the existingnode._clientaccess pattern inartifact.py— a follow-up refactoringartifact.pyonto the helper would be nice.- Pylint per-file ignores (
PLR0911/0912/0913) are scoped totasks/**with explanatory comments — appropriate. - Doc template fixes (pipe-escaping union types, newline flattening in table cells, empty-params guard) are real bug fixes for the generator, and the broken
./references/plugins/link prefix inreadme.mdx.j2is correctly fixed for where_plugin_index.mdxlands. - Tests are mock-based but assert the right seams (create kwargs passthrough, no-save on no-change, overwrite vs. skip on download). Gaps: no test for the
file_name__valuefallback lookup, and no test pinning the relationship-on-update behavior from issue 1.
Verdict
Approve with minor changes. Issues 1 and 2 are real behavioral surprises worth fixing (or at minimum documenting) before merge; the rest is polish. The core upload/download logic, idempotency handling, and SDK usage are correct — verified against infrahub-sdk 1.20.1 source.
🤖 Generated with Claude Code
Add upload_file_object and download_file_object Nornir task functions for managing files on Infrahub nodes that inherit from CoreFileObject. Upload supports SHA-1 checksum idempotency (skip re-upload when file is unchanged). Download returns base64-encoded binary content with UTF-8 text decoding for text MIME types, with optional local save. - Bump infrahub-sdk lower bound to >=1.19.0 - Add 13 unit tests covering create, update, skip, download, and error cases - Add plugin reference documentation and sidebar entry - Fix markdownlint, yamllint, and vale for CI compliance
- rename node_id/dest to object_id/save_to (use "object" not "node") - accept str | Path for file_path and save_to - add upload_from_bytes support via content/file_name args - forward **kwargs to client.create (allow_upsert, timeout, ...) - download_file_object is now idempotent: skip write when local SHA-1 matches server - move _get_client into nornir_infrahub.utils for reuse Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- plugin.mdx.j2 now escapes `|` in type names and flattens newlines in descriptions so types like `str | Path` and multi-line descriptions render as valid markdown tables (fixes markdown-lint MD055/MD056). - .vale/styles/Infrahub/sentence-case.yml: update stale "CoreFileObject node" exception to "CoreFileObject object" to match the renamed heading. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
#55 reworked the docs generator and added _plugin_index.mdx. Regenerate all plugin reference docs with the current template (escapes pipes in type names, flattens multi-line descriptions) and add the CoreFileObject section to the manually-maintained readme.mdx index. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…al_checksum Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ical .markdownlint.yml PR #71 originally added a root .markdownlint.json, but the repo's canonical config is .markdownlint.yml (docusaurus-aware: MD033/MD007/MD029/MD047 etc.). markdownlint-cli prefers .json over .yml, so our file shadowed the canonical one and #55's guide/topic docs failed markdown-lint after the rebase. Remove the .json and add the two rules the generated plugin reference tables need (MD012 multiple-blanks, MD060 table padding) to .markdownlint.yml. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- _lookup_existing_object now catches only NodeNotFoundError, so real API errors surface instead of silently triggering a create (review #1) - upload_file_object persists data attribute changes even when the file checksum matches, instead of dropping them on the skip path (review #2) - document **kwargs as (Any, optional) so generated docs show the right type and optionality (review #3) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- mark schema_mappings/group_mappings optional in the InfrahubInventory docstring so the generated table shows Required=No, matching the Optional[...] = None signature (review #4) - generate _plugin_index.mdx links relative to its own location instead of prepending references/plugins/, which produced broken doubled paths (review #5) - regenerate plugin reference docs; file_object_tasks.mdx now shows the **kwargs row as type Any / Required No (review #3) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- reject relationship and unknown keys in 'data' on update instead of silently dropping them; document that 'data' on update only sets attributes (review #1) - re-raise NodeNotFoundError when the caller passed an explicit object_id, so a typo'd UUID fails instead of creating a new node (review #2) - convert the SDK's IndexError on ambiguous filename lookup into a clean failed Result naming the conflict (review #5a) - detect Windows trailing separator on save_to (review #5b) - docstring notes: download memory/base64 footprint (review #5c) and **kwargs being create-only (review #5d) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
pyproject's [tool.ty.rules] disabled invalid-return-type and invalid-assignment project-wide while file_object.py also carried inline # type: ignore[...] comments at every relevant spot — the globals silenced those rules for all future code unnecessarily. Drop the two globals (keep the three that cover broader SDK-dynamic-type patterns) and move the existing return-type ignore on _validate_file_object_kind from the def line to the actual return line so ty honours it (review #4). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
upload_file_object's create path passes data={} to client.create when the
caller supplies neither data nor **kwargs, but the SDK rejects that with
'Either data or a list of keywords must be provided' — a bare upload
fails before upload_if_changed gets to set the real file_name.
Seed data['file_name'] from the upload source when there is nothing else
to send. upload_if_changed refreshes file_name from the actual content
before save, so the seed is invisible in the persisted object.
Discovered by the new CoreFileObject integration tests.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds tests/integration/test_file_object.py running against a live Infrahub spun up by #79's testcontainers harness. Loads a TestingTestFile schema that inherits from CoreFileObject plus a label attribute, and exercises: - upload_lifecycle: create → idempotent skip → update on content change - download_returns_content: bytes round-trip via base64 - download_skip_when_local_matches: second download is a no-op - data_attr_persists_on_skip: label is saved even when the file content is unchanged (verifies the metadata-on-skip behaviour) - explicit_object_id_not_found_fails: typo'd UUID fails cleanly - unknown_key_in_data_fails_on_update: bogus data key is rejected Run with 'pytest -m integration' (requires Docker). All 6 pass locally against the testcontainers Infrahub in ~70s. Closes the integration-testing follow-up referenced in the testing-status comment on PR #71 (post-#79). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
c19c27e to
9de8e3b
Compare
Integration tests landedFollowing up on the testing-status note — now that #79 is merged, this PR has been rebased onto Six integration tests, all passing locally against testcontainers in ~70s:
The run also surfaced a real pre-existing UX bug: Run integration tests with: |
Initial support for file upload
Summary
Add CoreFileObject support to nornir-infrahub with two new Nornir task functions:
upload_file_object— Upload a local file (or raw bytes) to an Infrahub node that inherits from CoreFileObject. Creates the node if it doesn't exist, updates it if the file changed, and skips the upload entirely when the SHA-1 checksum matches (idempotent). On update,dataattribute changes are persisted even when the file content is unchanged.download_file_object— Download file content from a CoreFileObject node. Returns base64-encoded binary plus a UTF-8textview for known text MIME types. Optionally writes to a localsave_topath; skips the write when the local file already matches the server checksum.Mirrors the CoreFileObject support added to the Ansible collection in opsmill/infrahub-ansible#317.
Changes
nornir_infrahub/plugins/tasks/file_object.py— new module with the two task functions, helpers (_validate_file_object_kind,_lookup_existing_object,_resolve_upload_source), and theTEXT_MIME_TYPESconstant. Idempotency and checksum comparison are delegated to the SDK vianode.upload_if_changed(source, name)andnode.matches_local_checksum(source).nornir_infrahub/plugins/tasks/__init__.py— export the two new functions.nornir_infrahub/utils.py— smallget_client(task)helper shared between the file_object tasks (ready forartifact.pyto adopt as a follow-up).tests/unit/test_file_object.py— 28 unit tests covering create, update, idempotent skip, attribute-only updates on the skip path, rejection of relationship and unknown keys indataon update, invalid kind, missing file, explicitobject_idmiss, ambiguous filename lookup, text/binary MIME download,save_todir-vs-file handling, local-match skip, and overwrite-on-mismatch.pyproject.toml— bumpinfrahub-sdkto>=1.20.1,<2(first release shippingupload_if_changed/matches_local_checksum/UploadResultfrom opsmill/infrahub-sdk-python#963); scopePLR0911/PLR0912/PLR0913ignores to the tasks directory; keep the three ty rule disables that cover broad SDK-dynamic-type patterns and drop the two that inline# type: ignorecomments already cover.docs/— auto-generated plugin reference for the two new tasks plus generator fixes the rebase surfaced (pipe-escaping in type names, multi-line description flattening, corrected relative path in_plugin_index.mdx,MD012/MD060disables folded into the canonical.markdownlint.ymland the redundant.markdownlint.jsonremoved).Usage
Behavior notes:
**kwargsare forwarded toclient.createand only apply on the create path; they are not used when updating an existing object.datadict only sets attributes; passing a relationship key or an unknown key fails the task instead of being silently dropped (use the SDK directly to change relationships on existing nodes).object_idfails the task rather than silently creating a new node; upsert-on-miss is still in effect forhfidand the filename fallback.Test plan
ruff check/ruff format --checkpasspylint10.00/10ty checkpassesmarkdown-lint/vale/ docs build pass on CI across Python 3.10–3.13tests/integration/test_file_object.pyadds aTesting.TestFileschema inheriting fromCoreFileObjectand covers upload lifecycle (create → idempotent skip → update), download round-trip, download-skip on local match, metadata-on-skip persistence, explicit-object_idnot-found failure, and unknown-data-key rejection. 6/6 pass locally against testcontainers in ~70s; excluded from CI by test: add integration test suite for InfrahubInventory (NORNIR-5) #79's-m 'not integration'addopts (Docker-only). Run withpytest tests/integration/test_file_object.py -m integration.