-
Notifications
You must be signed in to change notification settings - Fork 13
fix: emit loadable YAML for cardinality-many relationships #1306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: stable
Are you sure you want to change the base?
Changes from all commits
4b53d82
8fab991
49feedc
6f38733
8dc0961
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Fixed YAML output for populated cardinality-many relationships so it can be loaded by `infrahubctl object load`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,9 +7,11 @@ | |
|
|
||
| from __future__ import annotations | ||
|
|
||
| import asyncio | ||
| import json | ||
| import os | ||
| from typing import TYPE_CHECKING | ||
| from uuid import uuid4 | ||
|
|
||
| import pytest | ||
| import yaml | ||
|
|
@@ -23,6 +25,8 @@ | |
|
|
||
| if TYPE_CHECKING: | ||
| from collections.abc import Generator | ||
| from pathlib import Path | ||
| from typing import Any | ||
|
|
||
| from infrahub_sdk import InfrahubClient | ||
| from infrahub_sdk.node import InfrahubNode | ||
|
|
@@ -208,6 +212,65 @@ def test_create_missing_args(self, base_dataset: None) -> None: | |
| result = runner.invoke(app, ["object", "create", "TestingPerson"]) | ||
| assert result.exit_code != 0 | ||
|
|
||
| async def test_get_yaml_round_trips_attribute_many_relationship( | ||
| self, | ||
| base_dataset: None, | ||
| client: InfrahubClient, | ||
| schema_extension_01: dict[str, Any], | ||
| tmp_path: Path, | ||
| ) -> None: | ||
| """Round-trip cardinality-many attribute relationships through the CLI.""" | ||
| tags: list[InfrahubNode] = [] | ||
| rack: InfrahubNode | None = None | ||
| body_succeeded = False | ||
| try: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The try/finally block in this test hurts readability. The test should only be about what we expect from the system. I think using fixtures with a
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For instance something like this could to the trick. Fyi I have not tested it locally. |
||
| response = await client.schema.load(schemas=[schema_extension_01], wait_until_converged=True) | ||
| assert not response.errors | ||
|
|
||
| suffix = uuid4().hex | ||
| tag_names = [f"yaml-round-trip-{suffix}-one", f"yaml-round-trip-{suffix}-two"] | ||
| for tag_name in tag_names: | ||
| tag = await client.create(kind="BuiltinTag", name=tag_name) | ||
| await tag.save() | ||
| tags.append(tag) | ||
|
|
||
| rack_name = f"yaml-round-trip-rack-{suffix}" | ||
| created_rack = await client.create(kind="InfraRack", name=rack_name, tags=tags) | ||
| await created_rack.save() | ||
| rack = created_rack | ||
|
|
||
| get_result = await asyncio.to_thread( | ||
| runner.invoke, | ||
| app, | ||
| ["object", "get", "InfraRack", "--filter", f"name__value={rack_name}", "--output", "yaml"], | ||
| ) | ||
| assert get_result.exit_code == 0, f"object get failed: {get_result.output}" | ||
|
|
||
| object_file = tmp_path / "infra-rack.yaml" | ||
| object_file.write_text(get_result.stdout, encoding="utf-8") | ||
| load_result = await asyncio.to_thread(runner.invoke, app, ["object", "load", str(object_file)]) | ||
| assert load_result.exit_code == 0, f"object load failed: {load_result.output}" | ||
|
|
||
| fetched_rack = await client.get(kind="InfraRack", id=rack.id) | ||
| fetched_tags = fetched_rack._get_relationship_many(name="tags") | ||
| await fetched_tags.fetch() | ||
| assert sorted(peer.hfid or [] for peer in fetched_tags.peers) == sorted([[name] for name in tag_names]) | ||
| body_succeeded = True | ||
| finally: | ||
| cleanup_errors: list[Exception] = [] | ||
| if rack is not None: | ||
| try: | ||
| await rack.delete() | ||
| except Exception as exc: | ||
| cleanup_errors.append(exc) | ||
| for tag in reversed(tags): | ||
| try: | ||
| await tag.delete() | ||
| except Exception as exc: | ||
| cleanup_errors.append(exc) | ||
| if body_succeeded and cleanup_errors: | ||
| raise cleanup_errors[0] | ||
|
|
||
| def test_update_inline(self, base_dataset: None) -> None: | ||
| """Update a person's height using --set.""" | ||
| result = runner.invoke( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,11 +2,16 @@ | |
|
|
||
| from __future__ import annotations | ||
|
|
||
| import json | ||
| from unittest.mock import MagicMock | ||
|
|
||
| import yaml # pyright: ignore[reportMissingModuleSource] | ||
|
|
||
| from infrahub_sdk import InfrahubClient | ||
| from infrahub_sdk.config import Config | ||
| from infrahub_sdk.ctl.formatters.yaml import YamlFormatter | ||
| from infrahub_sdk.spec.object import InfrahubObjectFileData, RelationshipDataFormat, get_relationship_info | ||
| from tests.helpers.fixtures import read_fixture | ||
|
|
||
|
|
||
| def _make_mock_schema( | ||
|
|
@@ -327,7 +332,39 @@ def test_rel_cardinality_many_with_peers_uses_hfid(self) -> None: | |
|
|
||
| result = formatter.format_detail(node, schema) | ||
| parsed = yaml.safe_load(result) | ||
| assert parsed["spec"]["data"][0]["tags"] == {"data": ["tag1", "tag2"]} | ||
| assert parsed["spec"]["data"][0]["tags"] == ["tag1", "tag2"] | ||
|
|
||
| async def test_rel_cardinality_many_output_is_loadable_reference(self) -> None: | ||
| """Cardinality-many YAML output is accepted by the object loader as HFID references.""" | ||
| client = InfrahubClient(config=Config(address="http://mock")) | ||
| client.schema.set_cache(json.loads(read_fixture("schema_01.json")), branch="main") | ||
| schema = await client.schema.get(kind="CoreGraphQLQuery", branch="main") | ||
|
|
||
| peer1 = await client.create(kind="BuiltinTag", name="tag1") | ||
| peer2 = await client.create(kind="BuiltinTag", name="tag2") | ||
| node = await client.create( | ||
| kind="CoreGraphQLQuery", name="query1", query="query Test { ok }", tags=[peer1, peer2] | ||
| ) | ||
|
|
||
| parsed = yaml.safe_load(YamlFormatter().format_detail(node, schema)) | ||
| relationship_value = parsed["spec"]["data"][0]["tags"] | ||
| rel_info = await get_relationship_info( | ||
| client=client, | ||
| schema=schema, | ||
| name="tags", | ||
| value=relationship_value, | ||
| ) | ||
| errors = await InfrahubObjectFileData.validate_related_nodes( | ||
| client=client, | ||
| position=[1, "tags"], | ||
| rel_info=rel_info, | ||
| data=relationship_value, | ||
| ) | ||
|
|
||
| assert relationship_value == ["tag1", "tag2"] | ||
| assert rel_info.format == RelationshipDataFormat.MANY_REF | ||
| assert rel_info.is_reference | ||
| assert errors == [] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We try to avoid The reason is that we'd rather have a less isolated and slower test than one that looks like it protects against regressions but doesn't, because the mocks no longer match the real code. Existing tests such
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated in 49feedc: I replaced the mock-based regression setup with a real |
||
|
|
||
| def test_rel_multi_component_hfid(self) -> None: | ||
| """Multi-component HFID renders as a list.""" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a correct a minimal fix that describe what our documentation actually says.