Merge develop into infrahub-develop #489
Conversation
Co-authored-by: dgarros <[email protected]>
…ng _init_relationships (#456) * Add setting typename if it exists when calling _init_relationships on relationships that are already a RelatedNode * small unit test update * cover display_label and kind too * add changelog --------- Co-authored-by: Aaron McCarty <[email protected]>
…nsions (#463) * Fix load_from_disk method * Add unit test to cover file doesn't exist case * Add changelog * Add files to be ignored as part of the tests
Fix sync parallel filters
* Update dependencies Add init command Add testing changelog Add docs Add copier to all extras update lock file update lock file update lock file update lock file * update lock file * update lock file
* fix: improve cardinality many relationship fetch Signed-off-by: Fatih Acar <[email protected]>
Add check for empty list of schema
Prepare version 1.13.4
Fix vale lint command to exclude node_modules directory
respect ordering of files when loading
Prepare release 1.13.5
Create batch directly instead of using create_batch while fetching relationships
pass branch into count call
Merge stable into develop
…ort-to-protocols add support for NumerPool attributes to protocols
Finalize typing on ctl.schema
This change moves the `get_flat_value` function to methods of the `InfrahubNode` and `InfrahubNodeSync` classes. This allows to handle traversing relationships of cardinality one using a flat notation in addition to attributes. The `extract` method has also been moved as it needs to be async for regular nodes now.
WalkthroughThis update introduces new features, bug fixes, and enhancements across the SDK, CLI, and test suites. Notable changes include recursive flat key resolution in node classes, improved batch relationship fetching, a new CLI command for repository initialization, stricter YAML file handling, updated documentation, dependency additions, and expanded tests. Changes
Sequence Diagram(s)Repository Initialization CommandsequenceDiagram
participant User
participant CLI (infrahubctl)
participant Copier
participant FileSystem
User->>CLI (infrahubctl): Run 'repository init' with directory, template, data, etc.
CLI (infrahubctl)->>FileSystem: Check if template path exists
CLI (infrahubctl)->>FileSystem: Load YAML data if provided
CLI (infrahubctl)->>Copier: Run copy operation (async)
Copier->>FileSystem: Copy template files to target directory
Copier-->>CLI (infrahubctl): Return success or error
CLI (infrahubctl)-->>User: Print result or error
Node Flat Key ExtractionsequenceDiagram
participant Caller
participant InfrahubNode
participant RelatedNode
participant RelationshipManager
Caller->>InfrahubNode: get_flat_value("foo__bar__baz")
InfrahubNode->>InfrahubNode: Traverse attribute "foo"
alt Relationship (cardinality one)
InfrahubNode->>RelationshipManager: fetch related node
RelationshipManager->>RelatedNode: retrieve node data
RelatedNode->>InfrahubNode: get_flat_value("baz")
else Attribute
InfrahubNode->>InfrahubNode: Traverse attribute "bar"
InfrahubNode-->>Caller: Return value of "baz"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
the
type/documentation
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## infrahub-develop #489 +/- ##
====================================================
+ Coverage 70.92% 76.24% +5.31%
====================================================
Files 87 100 +13
Lines 7879 9032 +1153
Branches 1527 1731 +204
====================================================
+ Hits 5588 6886 +1298
+ Misses 1897 1670 -227
- Partials 394 476 +82
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 18 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
changelog/+add_numberpool_support_protocols.added.md (1)
1-1: Changelog style nitpickFor consistency with other entries, capitalise the first word and add a period:
Add support for NumberPool attributes in generated protocols.infrahub_sdk/yaml.py (1)
123-124: Address the FIXME comment regarding JSON filesThe comment indicates that
.jsonsupport should be removed since this is specifically for YAML files. Consider creating a separate issue to address this technical debt.Would you like me to create an issue to track the removal of JSON support from the YAML file loader?
infrahub_sdk/ctl/repository.py (1)
198-200: Consider using more specific exception typesThe code uses generic
Exceptionin except clauses. Consider catching more specific exceptions to handle different error scenarios appropriately:
- For YAML loading:
yaml.YAMLError,IOError,OSError- For copier execution: specific copier exceptions or
OSError- except Exception as exc: + except (yaml.YAMLError, IOError, OSError) as exc: typer.echo(f"Error loading YAML file: {exc}", err=True)- except Exception as e: + except (OSError, RuntimeError) as e: typer.echo(f"Error running copier: {e}", err=True)Also applies to: 216-218
infrahub_sdk/node/relationship.py (2)
165-166: Consider adding peer information to error messageThe error message could be more informative by including which peer lacks the required fields.
- if not peer.id or not peer.typename: - raise Error("Unable to fetch the peer, id and/or typename are not defined") + if not peer.id or not peer.typename: + raise Error(f"Unable to fetch the peer for relationship '{self.name}': id={peer.id}, typename={peer.typename}")Also applies to: 288-289
181-182: Document the side effect of batch executionThe batch execution updates peers through the store as a side effect. Consider adding a comment to clarify this behavior for future maintainers.
+ # Execute batch to populate the store with peer data async for _ in batch.execute(): passAlso applies to: 305-306
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
CHANGELOG.md(1 hunks)changelog/+add_numberpool_support_protocols.added.md(1 hunks)changelog/+batch.fixed.md(1 hunks)changelog/+branch-in-count.fixed.md(1 hunks)changelog/466.added.md(1 hunks)changelog/6882.fixed.md(1 hunks)docs/docs/infrahubctl/infrahubctl-repository.mdx(2 hunks)infrahub_sdk/client.py(5 hunks)infrahub_sdk/ctl/repository.py(2 hunks)infrahub_sdk/ctl/schema.py(6 hunks)infrahub_sdk/ctl/utils.py(1 hunks)infrahub_sdk/node/node.py(7 hunks)infrahub_sdk/node/related_node.py(2 hunks)infrahub_sdk/node/relationship.py(3 hunks)infrahub_sdk/protocols_base.py(0 hunks)infrahub_sdk/protocols_generator/constants.py(1 hunks)infrahub_sdk/utils.py(0 hunks)infrahub_sdk/yaml.py(1 hunks)pyproject.toml(4 hunks)tasks.py(1 hunks)tests/integration/test_node.py(1 hunks)tests/unit/ctl/test_repository_app.py(2 hunks)tests/unit/sdk/test_node.py(5 hunks)tests/unit/sdk/test_utils.py(0 hunks)tests/unit/sdk/test_yaml.py(2 hunks)
💤 Files with no reviewable changes (3)
- infrahub_sdk/utils.py
- tests/unit/sdk/test_utils.py
- infrahub_sdk/protocols_base.py
🧰 Additional context used
🧬 Code Graph Analysis (7)
infrahub_sdk/node/related_node.py (2)
infrahub_sdk/protocols_base.py (1)
get_kind(193-193)infrahub_sdk/node/node.py (1)
get_kind(173-174)
tests/integration/test_node.py (1)
infrahub_sdk/node/related_node.py (3)
peer(213-214)peer(260-261)typename(117-120)
infrahub_sdk/node/relationship.py (4)
infrahub_sdk/batch.py (1)
InfrahubBatch(55-89)infrahub_sdk/exceptions.py (1)
UninitializedError(156-157)infrahub_sdk/types.py (1)
Order(71-72)infrahub_sdk/node/related_node.py (4)
peer(213-214)peer(260-261)id(83-86)typename(117-120)
infrahub_sdk/ctl/schema.py (2)
infrahub_sdk/schema/__init__.py (5)
validate(101-102)load(287-299)load(712-724)check(321-335)check(746-760)infrahub_sdk/yaml.py (3)
data(147-150)payload(168-169)SchemaFile(166-169)
infrahub_sdk/node/node.py (6)
infrahub_sdk/utils.py (2)
compare_lists(124-135)generate_short_id(30-32)infrahub_sdk/node/attribute.py (2)
value(66-67)value(70-72)infrahub_sdk/protocols_base.py (7)
id(15-15)hfid(18-18)hfid(184-184)typename(33-33)display_label(30-30)RelatedNode(44-44)RelatedNodeSync(48-48)infrahub_sdk/node/related_node.py (13)
id(83-86)hfid(89-92)typename(117-120)kind(123-126)display_label(111-114)RelatedNode(182-226)fetch(204-210)fetch(251-257)peer(213-214)peer(260-261)get(216-226)get(263-273)RelatedNodeSync(229-273)infrahub_sdk/schema/main.py (3)
attribute_names(223-224)get_relationship(187-191)RelationshipCardinality(17-19)infrahub_sdk/node/relationship.py (3)
fetch(148-182)fetch(271-306)RelationshipManagerSync(223-343)
tests/unit/sdk/test_node.py (2)
tests/unit/sdk/conftest.py (5)
mock_schema_query_01(1824-1831)clients(37-42)location_schema(140-172)location_data01(375-412)client(32-33)infrahub_sdk/node/node.py (6)
InfrahubNode(452-1078)get_flat_value(1031-1061)get_flat_value(1657-1687)InfrahubNodeSync(1081-1704)extract(1063-1069)extract(1689-1695)
infrahub_sdk/yaml.py (1)
infrahub_sdk/exceptions.py (1)
FileNotValidError(164-167)
🪛 LanguageTool
docs/docs/infrahubctl/infrahubctl-repository.mdx
[style] ~67-~67: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ...Template to use for the new repository. Can be a local path or a git repository URL...
(MISSING_IT_THERE)
🔇 Additional comments (51)
infrahub_sdk/protocols_generator/constants.py (1)
25-25: Confirm downstream handling for newly-added'NumberPool'mappingMapping the new attribute kind to
"Integer"looks fine in isolation.
Please double-check that:• Any validation or enum logic relying on
ATTRIBUTE_KIND_MAP.keys()recognises'NumberPool'.
• Protocol-generation tests cover this kind to catch template regressions.
• Serialisation/deserialisation code paths treatNumberPoolconsistently as integers (e.g. no string casting surprises).If these items are already covered elsewhere, no further action needed.
changelog/+branch-in-count.fixed.md (1)
1-1: LGTM - Clear changelog entryThe changelog entry clearly describes the update to include branch parameters in count calls, ensuring branch-aware querying.
pyproject.toml (3)
3-3: Version update looks appropriateThe version bump from 1.13.2 to 1.13.5 aligns with the feature additions and fixes in this release.
48-48: Dependency addition for new repository init commandThe addition of the
copierdependency correctly supports the newinfrahubctl repository initcommand functionality.
73-73: Proper inclusion in extras groupsThe
copierdependency is correctly included in both thectlandallextras groups, ensuring it's available when needed.Also applies to: 85-85
infrahub_sdk/yaml.py (3)
126-128: Excellent error handling improvementThe explicit path existence check with a meaningful error message using
FileNotValidErrorsignificantly improves user experience when invalid paths are provided.
130-132: Good file extension filteringThe implementation correctly filters files by extension and silently skips irrelevant files, which is the appropriate behavior for a YAML loader.
134-137: Well-implemented recursive directory handlingThe recursive approach with sorting ensures consistent processing order and proper handling of nested directory structures. The sorting by name provides predictable behavior.
changelog/+batch.fixed.md (1)
1-1: Clear description of batch handling improvementThe changelog entry accurately describes the change to create new batches during relationship fetching instead of reusing existing ones, which should improve performance and reliability.
changelog/466.added.md (1)
1-1: Comprehensive description of new repository init commandThe changelog entry clearly describes the new
infrahubctl repository initcommand and appropriately references the template repository it uses.changelog/6882.fixed.md (1)
1-1: LGTM - Clear changelog entryThe changelog entry clearly describes the fix for flat notation value lookup with relationships of cardinality one.
tests/integration/test_node.py (1)
86-86: LGTM - Enhanced test coverageGood addition to verify the
typenameattribute of related nodes, which aligns with the enhanced metadata handling introduced in this PR.tasks.py (1)
204-204: LGTM - Sensible exclusion of node_modulesGood improvement to exclude
node_modulesdirectories from Vale linting, which avoids checking third-party documentation files.infrahub_sdk/node/related_node.py (2)
42-42: LGTM - Proper initializationThe
_kindattribute is properly initialized to None, following the pattern of other metadata attributes.
122-126: LGTM - Well-implemented kind propertyThe
kindproperty follows the established pattern of delegating to the peer when available, otherwise returning the stored value. The implementation is consistent with other similar properties in this class.tests/unit/sdk/test_yaml.py (3)
3-3: LGTM - Required import for exception testingAppropriate addition of pytest import for the new exception testing functionality.
5-6: LGTM - Proper imports for new testGood addition of the required imports for the new test case covering error handling scenarios.
51-54: LGTM - Good error handling test coverageExcellent addition of a test case that verifies proper error handling when attempting to load YAML files from a non-existent directory. The test correctly uses
pytest.raisesand validates the error message content.infrahub_sdk/ctl/utils.py (1)
190-192: LGTM! Good defensive programming.The addition of an early exit when no valid files are found prevents unexpected behavior and provides clear feedback to users. The error message is informative and the exit code is appropriate.
CHANGELOG.md (1)
14-35: LGTM! Standard changelog documentation.The changelog entries follow the established format and properly document the historical releases with clear descriptions and issue references.
infrahub_sdk/client.py (3)
827-827: Good addition of branch parameter to count methodThe addition of the
branchparameter to the count method ensures that count queries respect the branch context, which is essential for accurate counting in a branch-aware system.Also applies to: 832-832, 849-849
832-832: Improved code clarity with keyword argumentsUsing keyword arguments for
process_pagecalls improves code readability and makes the parameter passing more explicit.Also applies to: 849-849, 1997-1997, 2014-2014
1950-1950: Consistent branch parameter addition in sync clientThe sync client implementation correctly mirrors the async client's changes by adding the
branchparameter to the count method, maintaining consistency between both implementations.Also applies to: 1992-1992
docs/docs/infrahubctl/infrahubctl-repository.mdx (1)
22-22: Well-documented new commandThe documentation for the new
initcommand is comprehensive and follows the existing documentation pattern consistently. All options are clearly described with appropriate defaults.Also applies to: 51-73
infrahub_sdk/node/relationship.py (2)
163-182: Efficient batched fetching implementationThe refactoring from individual peer fetches to batched fetching by typename is a solid performance improvement. The use of
Order(disable=True)further optimizes the queries.
293-293: Clear comment about batch creation differenceGood documentation explaining why the sync version doesn't need a new batch instance due to the absence of semaphore-based concurrency control.
infrahub_sdk/ctl/schema.py (8)
35-35: LGTM - Function signature updated for better type safety.The type hint change from
list[dict]tolist[SchemaFile]improves type safety and accurately reflects that the function expectsSchemaFileinstances rather than raw dictionaries.
39-39: Good refactoring to use the payload property.Using
schema_file.payloadinstead of directcontentaccess provides better encapsulation and ensures a dictionary is always returned (falling back to{}if content isNone).
51-51: LGTM - Consistent type hint improvement.The updated type hint correctly reflects that the function expects
SchemaFileinstances, maintaining consistency with other function signature updates in this file.
90-90: LGTM - Safe direct access after key existence check.The change from
response.get("errors")toresponse["errors"]is appropriate since line 89 already verifies the "errors" key exists in the response.
100-102: Consistent refactoring to use payload property.The function signature update and switch to
payloadaccess maintain consistency with the broader refactoring in this file. The payload property provides safer access to schema data.
125-125: Consistent payload usage in schema loading.The change to use
item.payloadmaintains consistency with the refactoring throughout this file and ensures proper data access.
173-173: Consistent payload usage in schema checking.The change to use
item.payloadaligns with the refactoring pattern and ensures consistent data access across schema operations.
176-176: Good defensive programming for None response handling.Using
response or {}safely handles the case whereclient.schema.checkmight returnNoneas the second tuple element, preventing potential AttributeError when accessing response keys.tests/unit/ctl/test_repository_app.py (4)
3-4: LGTM - Appropriate imports for new test functionality.The addition of
tempfileandPathimports supports the new test methods that need to create temporary directories and files for testing the repository init command.
8-8: LGTM - YAML import needed for test data handling.The
yamlimport is required for the new tests that need to create and read YAML configuration files for the repository init command testing.
329-361: Well-structured integration test for repository init command.The test properly uses context managers for resource cleanup, creates realistic test data, and verifies both the command execution and the expected output structure. The assertions cover the key aspects of the repository initialization functionality.
A few observations:
- Good use of temporary files and directories for isolation
- Proper verification of both the copied answers and the generated project structure
- The removal of
_src_pathfrom copied answers is appropriate as it's an internal copier field
363-397: Comprehensive test for local template functionality.This test effectively covers the local template scenario with:
- Proper setup of a minimal copier template structure
- Creation of Jinja template files for testing rendering
- Verification of both command success and output correctness
- Good use of temporary directories for test isolation
The test demonstrates thorough coverage of the template rendering workflow and validates that the generated content matches expectations.
tests/unit/sdk/test_node.py (4)
199-205: LGTM! Good test coverage expansion.The additional parametrized test case appropriately tests relationship initialization with different metadata field formats (
__typenamevskind/display_label), which aligns with the enhanced relationship node handling mentioned in the summary.
240-242: LGTM! Proper verification of relationship metadata.These assertions correctly verify that relationship nodes now carry the additional metadata fields (
typename,kind,display_label) as mentioned in the enhanced summary, ensuring the new functionality works as expected.
885-897: LGTM! Mock response properly simulates batched fetching.The additional intermediate mock response (
response2) with count but no edges correctly simulates the new batched relationship fetching behavior, ensuring tests cover the pagination scenarios mentioned in the summary.
963-2008: Excellent test coverage for new flat key resolution functionality.Both
test_get_flat_valueandtest_node_extractprovide comprehensive testing of the new methods with proper:
- Testing of different key formats and separators
- Error handling for multi-cardinality relationships
- Coverage of both async and sync variants
- HTTP mocking that matches the new batched fetching behavior
The tests align well with the functionality described in the relevant code snippets from
infrahub_sdk/node/node.py.infrahub_sdk/node/node.py (9)
11-11: LGTM!The removal of
get_flat_valuefrom the utils import is consistent with its reimplementation as a method within the node classes.
405-409: Good fix for handling zero values correctly.The explicit
is not Nonechecks are more robust than falsiness checks, correctly handling cases whereoffsetorlimitis 0.
502-512: Clean refactoring using dictionary comprehension.The dictionary comprehension efficiently includes only non-None values for the relationship data fields, making the code more concise and maintainable.
1031-1061: Well-implemented recursive flat value resolution.The async
get_flat_valuemethod correctly handles both attributes and relationships with proper error handling and cardinality validation. The recursive approach for relationship traversal is appropriate.
1063-1069: Simple and effective extract method implementation.The method correctly delegates to
get_flat_valuefor each parameter, maintaining the async pattern throughout.
1131-1141: Consistent refactoring in sync class.The dictionary comprehension in
InfrahubNodeSynccorrectly mirrors the implementation inInfrahubNode, maintaining consistency between async and sync versions.
1528-1529: Good defensive programming with safer dictionary access.Using
.get()withNonedefault prevents potential KeyError exceptions when accessing nested dictionary keys, improving robustness.Also applies to: 1535-1536
1657-1687: Correct synchronous implementation of get_flat_value.The synchronous version properly mirrors the async implementation, correctly handling both attributes and relationships without async/await.
1689-1695: Consistent synchronous extract implementation.The method correctly mirrors the async version, maintaining consistency across the codebase.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Refactor
Chores