Conversation
WalkthroughThis pull request makes the Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (2)
infrahub_sync/adapters/slurpitsync.py (1)
178-180: Consider refactoring duplicate guard logic and using structured logging.This guard pattern is duplicated across 7 adapter files with identical logic and messaging. Additionally, using
print()is inconsistent with production logging best practices.Suggestions:
- Extract common guard logic into a shared utility function or mixin method:
def should_skip_unmapped_element(element: SchemaMappingModel, adapter_type: str) -> bool: """Check if element should be skipped due to missing mapping.""" if not element.mapping: logger.info("No mapping defined for '%s' in %s adapter, skipping...", element.name, adapter_type) return True return False
- Use structured logging instead of
print():- if not element.mapping: - print(f"No mapping defined for '{element.name}', skipping...") - continue + if not element.mapping: + logger.info("No mapping defined for '%s', skipping...", element.name) + continueThis applies to all adapter files:
slurpitsync.py,nautobot.py,prometheus.py,netbox.py,genericrestapi.py, andaci.py.infrahub_sync/adapters/infrahub.py (1)
72-73: Consider using structured logging instead of print().While the added log message provides useful visibility into successful fallback resolution, using
print()is inconsistent with production logging best practices. The file doesn't currently importlogging, but structured logging would be more appropriate for this diagnostic information.Consider importing and using the logging module:
+import logging + +logger = logging.getLogger(__name__) + def resolve_peer_node( ... ) -> InfrahubNodeSync | None: ... if not peer_node and fallback: - print(f"Unable to find {rel_schema.peer} [{key}] in Store - Fallback to Infrahub") + logger.info("Unable to find %s [%s] in Store - Fallback to Infrahub", rel_schema.peer, key) peer_node = client.get(id=key, kind=rel_schema.peer, populate_store=True) if not peer_node: - print(f"Unable to find {rel_schema.peer} [{key}] - Ignored") + logger.warning("Unable to find %s [%s] - Ignored", rel_schema.peer, key) else: - print(f"Found it with kind {peer_node.get_kind()} using Infrahub client") + logger.info("Found peer with kind %s using Infrahub client", peer_node.get_kind())This would also apply to other
print()statements in this function and throughout the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
infrahub_sync/__init__.py(1 hunks)infrahub_sync/adapters/aci.py(1 hunks)infrahub_sync/adapters/genericrestapi.py(1 hunks)infrahub_sync/adapters/infrahub.py(1 hunks)infrahub_sync/adapters/ipfabricsync.py(2 hunks)infrahub_sync/adapters/nautobot.py(1 hunks)infrahub_sync/adapters/netbox.py(1 hunks)infrahub_sync/adapters/prometheus.py(1 hunks)infrahub_sync/adapters/slurpitsync.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pull Request on main/stable
infrahub_sync/adapters/ipfabricsync.py
[error] 62-62: E303: Too many blank lines (2) before line 62. Found 1 error. 1 fixable with the --fix option. Command: poetry run ruff check .
🔇 Additional comments (6)
infrahub_sync/__init__.py (1)
48-48: LGTM! Mapping field is now optional.This change correctly makes the
mappingfield optional inSchemaMappingModel, which aligns with the PR objective of allowing schema mapping entries without a source model mapping.infrahub_sync/adapters/nautobot.py (1)
64-66: Guard implementation is correct.The guard correctly prevents accessing
element.mappingwhen it'sNone, avoiding runtime errors on line 69 whereelement.mapping.split(".")is called.Note: See the comment on
slurpitsync.pyregarding refactoring duplicate guard logic across adapters.infrahub_sync/adapters/prometheus.py (1)
473-475: Guard implementation is correct.The guard correctly prevents accessing
element.mappingwhen it'sNone, ensuring safe access on line 477.Note: See the comment on
slurpitsync.pyregarding refactoring duplicate guard logic across adapters.infrahub_sync/adapters/netbox.py (1)
68-70: Guard implementation is correct.The guard correctly prevents accessing
element.mappingwhen it'sNone, avoiding runtime errors on line 73 whereelement.mapping.split(".")is called.Note: See the comment on
slurpitsync.pyregarding refactoring duplicate guard logic across adapters.infrahub_sync/adapters/genericrestapi.py (1)
144-146: Guard implementation is correct.The guard correctly prevents accessing
element.mappingwhen it'sNone, ensuring safe access on line 149.Note: See the comment on
slurpitsync.pyregarding refactoring duplicate guard logic across adapters.infrahub_sync/adapters/ipfabricsync.py (1)
78-80: LGTM! Guard correctly implements optional mapping.The guard properly skips processing when no mapping is defined, aligning with the PR objective to allow optional mapping entries in schema_mapping. The placement ensures safe access to
element.mappingon line 81.
Deploying infrahub-sync with
|
| Latest commit: |
2fa9220
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bbb8b17e.infrahub-sync.pages.dev |
| Branch Preview URL: | https://bkr-optional-mapping.infrahub-sync.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
examples/device42_to_infrahub/genericrestapi/sync_models.py (1)
36-44: Critical: Mutable default argument causes shared state across instances.Line 41 uses
[]as the default value fortags. This list is created once at class definition and shared across all instances that don't explicitly provide a value, leading to unexpected behavior when modified.Apply this diff to fix:
- tags: list[str] | None = [] + tags: list[str] | None = NoneIf an empty list is required when
tagsis None, initialize it in__init__or__post_init__instead.examples/peering-manager_to_infrahub/peeringmanager/sync_models.py (1)
7-19: Consider more specific exception handling and add logging for the fallback.The broad
except Exceptionon line 14 silently catches all errors during plugin resolution, which could mask configuration issues or legitimate errors. Users may not realize they're using the fallbackDiffSyncModelinstead of the intended plugin-specific model class.Consider this improvement:
# Load model class dynamically at runtime (honor adapters_path, safe fallback) try: _loader = PluginLoader.from_env_and_args(adapter_paths=[]) _spec = "peeringmanager" _ModelBaseClass = _loader.resolve(_spec, default_class_candidates=("Model",)) -except Exception: +except Exception as e: # Fallback: use DiffSyncModel to avoid import-time failure + import logging + logging.getLogger(__name__).debug( + "Could not resolve model base class for '%s', falling back to DiffSyncModel: %s", + "peeringmanager", + e, + ) from diffsync import DiffSyncModel as _FallbackModel _ModelBaseClass = _FallbackModelexamples/device42_to_infrahub/infrahub/sync_models.py (1)
36-44: Fix the generator to useField(default_factory=list)instead of mutable defaults.The mutable default
[]for optional list fields is confirmed unsafe in Pydantic v2, which DiffSyncModel requires. The issue affects all auto-generatedsync_models.pyfiles (dozens across examples).Since these files carry the header "AUTO-GENERATED FILE, DO NOT MODIFY" with regeneration command
infrahub-sync generate, manual fixes will be lost on next regeneration. The root cause is in the generator code—it should emit:tags: list[str] = Field(default_factory=list) # instead of: tags: list[str] | None = []This ensures each instance gets a fresh list and prevents shared mutable state across instances, which is required for Pydantic v2 compliance.
examples/netbox_to_infrahub/infrahub/sync_models.py (2)
125-136: InfraInterfaceL2L3: missing attribute in _attributes tuple.untagged_vlan is defined but not listed in _attributes, so it won’t sync/diff.
- _attributes = ("tagged_vlan", "tags", "l2_mode", "mac_address", "description", "interface_type", "mgmt_only") + _attributes = ("tagged_vlan", "untagged_vlan", "tags", "l2_mode", "mac_address", "description", "interface_type", "mgmt_only")Note: adjust the generator to persist this.
54-55: Fix the generator to emit None instead of empty lists for mutable default list attributes.The mutable default list antipattern is confirmed across all generated sync_models.py files. Attributes like
tags,tagged_vlan,export_rt,import_rt, etc., are initialized with[], creating shared class-level lists that leak state across instances. Fix the generator to emitNoneas the default value instead.Example:
- tags: list[str] | None = [] + tags: list[str] | None = NoneThis pattern affects 60+ attributes across multiple integrations: netbox_to_infrahub, slurpit_to_infrahub, peering-manager_to_infrahub, nautobot-v2_to_infrahub, device42_to_infrahub, and infrahub_to_peering-manager.
examples/nautobot-v2_to_infrahub/nautobot/sync_models.py (2)
90-102: Identifiers should not be Optional.These fields are listed in _identifiers but typed Optional with None defaults, which can break identity/merging.
@@ class InfraDevice(_ModelBaseClass): - name: str | None = None + name: str @@ - organization: str | None = None + organization: str@@ class InfraIPAddress(_ModelBaseClass): - ip_prefix: str | None = None + ip_prefix: str@@ class InfraPlatform(_ModelBaseClass): - manufacturer: str | None = None + manufacturer: str@@ class InfraPrefix(_ModelBaseClass): - ip_namespace: str | None = None + ip_namespace: str@@ class InfraRouteTarget(_ModelBaseClass): - organization: str | None = None + organization: strIf any of these truly are optional in source, consider removing them from _identifiers instead.
Also applies to: 168-177, 204-213, 232-244, 279-286
154-155: Type/default mismatch for mgmt_only.Optional[bool] with default False is confusing. Use a plain bool with default False, or Optional[bool] defaulting to None.
- mgmt_only: bool | None = False + mgmt_only: bool = False
♻️ Duplicate comments (4)
examples/netbox_to_infrahub/netbox/sync_models.py (4)
8-18: Same loader fallback concern as infrahub models.Catching Exception is too broad; prefer PluginLoadError. See prior diff; apply here too.
125-136: InfraInterfaceL2L3: add untagged_vlan to _attributes.Mirror the infrahub fix; otherwise the field won’t be synced.
- _attributes = ("tagged_vlan", "tags", "l2_mode", "mac_address", "description", "interface_type", "mgmt_only") + _attributes = ("tagged_vlan", "untagged_vlan", "tags", "l2_mode", "mac_address", "description", "interface_type", "mgmt_only")
54-55: Shared mutable defaults (lists).Same issue as the infrahub file; fix in the generator (use None or per-instance list creation).
Also applies to: 69-70, 82-83, 101-102, 132-135, 164-165, 179-180, 221-224, 237-238
94-104: Optional identifier and Optional[bool] default.Make InfraDevice.name non-optional if required; make mgmt_only a plain bool.
🧹 Nitpick comments (14)
infrahub_sync/adapters/ipfabricsync.py (1)
77-79: Guard logic correctly implements optional mapping.The guard properly checks for missing mappings and skips processing with a clear message, preventing errors when
element.mappingis used on line 80. This aligns with the PR objective to allow schema entries without source model mappings.Note: While
print()is consistent with existing logging in this file (lines 86, 90), consider migrating to structured logging (e.g., Python'sloggingmodule) for better observability in production environments.examples/peeringdb_to_infrahub/infrahub/sync_models.py (1)
8-18: Consider narrowing the exception scope in the generatorThe broad
except Exception:handler on line 14 will silently catch all exceptions during base class resolution, potentially masking import errors, configuration issues, or bugs in the PluginLoader. While this ensures the module always loads, it may hide real problems that should be surfaced.Since this is auto-generated code (lines 21-25 warn modifications will be lost), consider updating the generator to:
- Catch specific exceptions (e.g.,
PluginLoadError,ImportError)- Log warnings when falling back to
DiffSyncModelso users are aware of resolution failuresNote: Changes should be made to the code generator that produces this file, not to this file directly.
examples/peeringdb_to_infrahub/genericrestapi/sync_models.py (1)
8-18: Consider narrowing the exception scope in the generatorThe broad
except Exception:handler on line 14 will silently catch all exceptions during base class resolution, potentially masking import errors, configuration issues, or bugs in the PluginLoader. While this ensures the module always loads, it may hide real problems that should be surfaced.Since this is auto-generated code (lines 21-25 warn modifications will be lost), consider updating the generator to:
- Catch specific exceptions (e.g.,
PluginLoadError,ImportError)- Log warnings when falling back to
DiffSyncModelso users are aware of resolution failuresNote: Changes should be made to the code generator that produces this file, not to this file directly. This same pattern appears in multiple generated sync_models.py files.
examples/custom_adapter/mockdb/sync_models.py (1)
7-18: Consider adding logging when fallback is triggered.The broad
except Exception:silently falls back toDiffSyncModelwithout any indication that the dynamic resolution failed. In production, this could mask configuration issues or plugin loading failures.Consider adding a warning log in the except block:
except Exception: # Fallback: use DiffSyncModel to avoid import-time failure + import logging + logging.getLogger(__name__).warning( + "Failed to dynamically load model base class from spec %r, falling back to DiffSyncModel", + _spec if '_spec' in locals() else 'unknown' + ) from diffsync import DiffSyncModel as _FallbackModel _ModelBaseClass = _FallbackModelNote: The dynamic loading happens at import time, which means
PluginLoader.from_env_and_args()executes when this module is first imported. This is by design for auto-generated code, but be aware it may impact startup performance or behavior in constrained environments.examples/custom_adapter/infrahub/sync_models.py (1)
7-18: Consider adding logging when fallback is triggered.The broad
except Exception:silently falls back toDiffSyncModelwithout any indication that the dynamic resolution failed. This could mask configuration or loading issues in production.Consider adding a warning log in the except block:
except Exception: # Fallback: use DiffSyncModel to avoid import-time failure + import logging + logging.getLogger(__name__).warning( + "Failed to dynamically load model base class from spec %r, falling back to DiffSyncModel", + _spec if '_spec' in locals() else 'unknown' + ) from diffsync import DiffSyncModel as _FallbackModel _ModelBaseClass = _FallbackModelNote: This file uses
_spec = "infrahub"(a module name), while the mockdb version uses a file path. This difference appears intentional for different adapter resolution strategies, but worth confirming both specs are correct for their respective contexts.examples/infrahub_to_peering-manager/infrahub/sync_models.py (1)
8-18: Narrow the exception and surface a minimal diagnostic.Catching Exception hides real import/config errors. Prefer a narrower exception set (e.g., ImportError, AttributeError, PluginLoadError if available) and emit a one-line warning to stderr for visibility.
Example:
-try: +try: _loader = PluginLoader.from_env_and_args(adapter_paths=[]) _spec = "infrahub" _ModelBaseClass = _loader.resolve(_spec, default_class_candidates=("Model",)) -except Exception: +except (ImportError, AttributeError): # Fallback: use DiffSyncModel to avoid import-time failure from diffsync import DiffSyncModel as _FallbackModel - _ModelBaseClass = _FallbackModel + _ModelBaseClass = _FallbackModel + # Optional: basic diagnostic without hard dependency on logging + import sys + print(f"[infrahub] Falling back to DiffSyncModel (spec={_spec})", file=sys.stderr)examples/observium_to_infrahub/genericrestapi/sync_models.py (1)
8-18: Tighten error handling on dynamic base resolution.Same pattern: prefer catching specific import/resolve errors and emitting a brief warning to aid debugging while keeping the safe fallback.
-except Exception: +except (ImportError, AttributeError): # Fallback: use DiffSyncModel to avoid import-time failure from diffsync import DiffSyncModel as _FallbackModel _ModelBaseClass = _FallbackModelexamples/peering-manager_to_infrahub/infrahub/sync_models.py (1)
8-18: Reduce catch-all to targeted exceptions and add a minimal diagnostic.Mirror the recommended loader handling used elsewhere: catch import/resolve errors only, and print a short notice when falling back.
examples/observium_to_infrahub/infrahub/sync_models.py (1)
8-18: Constrain the exception type for loader fallback.Catch import/resolve failures instead of Exception to avoid masking real errors; optional one-line stderr note helps troubleshooting.
examples/netbox_to_infrahub/infrahub/sync_models.py (3)
8-18: Narrow exception handling for loader fallback.Catching Exception hides unrelated import-time bugs. Catch PluginLoadError explicitly and re-raise others. Based on infrahub_sync/plugin_loader.py.
-from infrahub_sync.plugin_loader import PluginLoader +from infrahub_sync.plugin_loader import PluginLoader, PluginLoadError @@ -try: +try: @@ - _ModelBaseClass = _loader.resolve(_spec, default_class_candidates=("Model",)) -except Exception: + _ModelBaseClass = _loader.resolve(_spec, default_class_candidates=("Model",)) +except PluginLoadError: # Fallback: use DiffSyncModel to avoid import-time failure from diffsync import DiffSyncModel as _FallbackModel @@ - _ModelBaseClass = _FallbackModel + _ModelBaseClass = _FallbackModel
94-104: Identifier typed optional: consider making required.InfraDevice._identifiers includes "name" but it's annotated as Optional[str]. Prefer non-optional for identifiers to reflect required semantics (unless base class supports None identifiers).
If acceptable, change to:
- name: str | None = None + name: strVerify other classes for Optional identifiers. Would you like a script to scan and report these?
130-131: Boolean optional defaulted to False.mgmt_only is Optional[bool] but default False. Make it a plain bool or default None.
- mgmt_only: bool | None = False + mgmt_only: bool = Falseexamples/nautobot-v2_to_infrahub/nautobot/sync_models.py (2)
14-18: Narrow the exception; avoid masking real import bugs.Catching Exception will hide unrelated errors during import. Catch plugin-resolution failures explicitly.
-from infrahub_sync.plugin_loader import PluginLoader +from infrahub_sync.plugin_loader import PluginLoader, PluginLoadError @@ -except Exception: +except (PluginLoadError, ImportError):(Keep broad catch only if you also log/emit the traceback.) Based on plugin_loader.py.
9-9: Nit: let PluginLoader read env by default.Passing [] is equivalent to None here; simplify.
- _loader = PluginLoader.from_env_and_args(adapter_paths=[]) + _loader = PluginLoader.from_env_and_args()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
examples/aci_to_infrahub/aci/__init__.py(1 hunks)examples/aci_to_infrahub/aci/sync_models.py(5 hunks)examples/aci_to_infrahub/infrahub/__init__.py(1 hunks)examples/aci_to_infrahub/infrahub/sync_models.py(5 hunks)examples/custom_adapter/custom_adapter_src/custom_adapter.py(1 hunks)examples/custom_adapter/infrahub/sync_models.py(2 hunks)examples/custom_adapter/mockdb/sync_models.py(2 hunks)examples/device42_to_infrahub/genericrestapi/sync_models.py(4 hunks)examples/device42_to_infrahub/infrahub/sync_models.py(4 hunks)examples/infrahub_to_peering-manager/infrahub/sync_models.py(7 hunks)examples/infrahub_to_peering-manager/peeringmanager/sync_models.py(7 hunks)examples/librenms_to_infrahub/genericrestapi/sync_models.py(5 hunks)examples/librenms_to_infrahub/infrahub/sync_models.py(5 hunks)examples/nautobot-v2_to_infrahub/infrahub/sync_adapter.py(1 hunks)examples/nautobot-v2_to_infrahub/infrahub/sync_models.py(25 hunks)examples/nautobot-v2_to_infrahub/nautobot/sync_adapter.py(1 hunks)examples/nautobot-v2_to_infrahub/nautobot/sync_models.py(25 hunks)examples/netbox_to_infrahub/infrahub/sync_adapter.py(1 hunks)examples/netbox_to_infrahub/infrahub/sync_models.py(18 hunks)examples/netbox_to_infrahub/netbox/sync_adapter.py(1 hunks)examples/netbox_to_infrahub/netbox/sync_models.py(18 hunks)examples/observium_to_infrahub/genericrestapi/sync_models.py(4 hunks)examples/observium_to_infrahub/infrahub/sync_models.py(4 hunks)examples/peering-manager_to_infrahub/infrahub/sync_models.py(9 hunks)examples/peering-manager_to_infrahub/peeringmanager/sync_models.py(9 hunks)examples/peeringdb_to_infrahub/genericrestapi/sync_models.py(2 hunks)examples/peeringdb_to_infrahub/infrahub/sync_models.py(2 hunks)infrahub_sync/adapters/ipfabricsync.py(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- examples/aci_to_infrahub/infrahub/init.py
- examples/custom_adapter/custom_adapter_src/custom_adapter.py
- examples/aci_to_infrahub/aci/init.py
- examples/netbox_to_infrahub/infrahub/sync_adapter.py
🧰 Additional context used
🧬 Code graph analysis (22)
examples/device42_to_infrahub/genericrestapi/sync_models.py (1)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)
examples/peeringdb_to_infrahub/infrahub/sync_models.py (1)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)
examples/librenms_to_infrahub/genericrestapi/sync_models.py (1)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)
examples/librenms_to_infrahub/infrahub/sync_models.py (1)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)
examples/netbox_to_infrahub/infrahub/sync_models.py (1)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)
examples/aci_to_infrahub/infrahub/sync_models.py (1)
infrahub_sync/plugin_loader.py (2)
PluginLoader(46-481)from_env_and_args(68-104)
examples/infrahub_to_peering-manager/infrahub/sync_models.py (1)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)
examples/custom_adapter/infrahub/sync_models.py (1)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)
examples/peeringdb_to_infrahub/genericrestapi/sync_models.py (1)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)
examples/nautobot-v2_to_infrahub/infrahub/sync_models.py (2)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)examples/nautobot-v2_to_infrahub/nautobot/sync_models.py (1)
InfraInterfaceL2L3(137-163)
examples/peering-manager_to_infrahub/infrahub/sync_models.py (2)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)examples/peering-manager_to_infrahub/peeringmanager/sync_models.py (2)
InfraAutonomousSystem(47-69)InfraIXPConnection(132-154)
examples/netbox_to_infrahub/netbox/sync_adapter.py (1)
examples/netbox_to_infrahub/netbox/sync_models.py (1)
InfraIPAddress(109-119)
examples/custom_adapter/mockdb/sync_models.py (1)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)
examples/nautobot-v2_to_infrahub/nautobot/sync_models.py (2)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)examples/nautobot-v2_to_infrahub/infrahub/sync_models.py (1)
InfraInterfaceL2L3(137-163)
examples/nautobot-v2_to_infrahub/nautobot/sync_adapter.py (1)
examples/nautobot-v2_to_infrahub/infrahub/sync_models.py (1)
InfraIPAddress(166-179)
examples/infrahub_to_peering-manager/peeringmanager/sync_models.py (1)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)
examples/observium_to_infrahub/genericrestapi/sync_models.py (1)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)
examples/device42_to_infrahub/infrahub/sync_models.py (1)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)
examples/aci_to_infrahub/aci/sync_models.py (1)
infrahub_sync/plugin_loader.py (2)
PluginLoader(46-481)from_env_and_args(68-104)
examples/peering-manager_to_infrahub/peeringmanager/sync_models.py (2)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)examples/peering-manager_to_infrahub/infrahub/sync_models.py (2)
InfraAutonomousSystem(47-69)InfraIXPConnection(132-154)
examples/observium_to_infrahub/infrahub/sync_models.py (1)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)
examples/netbox_to_infrahub/netbox/sync_models.py (1)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)
🔇 Additional comments (34)
examples/nautobot-v2_to_infrahub/nautobot/sync_adapter.py (1)
16-16: LGTM! Auto-generated import reordering.The repositioning of
InfraIPAddressafterInfraInterfaceL2L3is expected for an auto-generated file and has no functional impact. Import order does not affect runtime behavior in this context.examples/device42_to_infrahub/genericrestapi/sync_models.py (2)
7-19: LGTM: Dynamic base class resolution with safe fallback.The try/except block correctly attempts runtime resolution of the model base class via PluginLoader and falls back to DiffSyncModel on any exception, ensuring import-time stability.
47-55: LGTM: Description field added correctly.The new optional
descriptionattribute uses an immutable default value and is properly declared in_attributes.examples/peering-manager_to_infrahub/peeringmanager/sync_models.py (3)
37-44: Verify that empty_attributesis intentional for reference-only model.The
OrganizationProvidermodel has an empty_attributestuple, which aligns with the PR objective of allowing schema_mapping entries without source model mapping. This appears to be a reference-only model that other models (likeInfraAutonomousSystem.organization) can reference.Please confirm that this model is intended to serve only as a static reference target without data synchronization from a source system.
50-58: LGTM! Formatting improvement enhances readability.The multiline formatting of the
_attributestuple improves readability without changing functionality.
135-143: LGTM! Consistent formatting improvement.The multiline formatting of the
_attributestuple maintains consistency with the formatting improvements elsewhere in the file.examples/peeringdb_to_infrahub/infrahub/sync_models.py (2)
3-3: LGTM: Import cleanupRemoving the unused
Listimport is appropriate since onlyAnyis needed for the type annotations in this generated file.
26-36: LGTM: Model class definitionThe model class correctly uses the dynamically resolved
_ModelBaseClassand maintains proper structure with identifiers, attributes, and typed fields.examples/peeringdb_to_infrahub/genericrestapi/sync_models.py (2)
3-3: LGTM: Import cleanupRemoving the unused
Listimport is appropriate since onlyAnyis needed for the type annotations in this generated file.
26-36: LGTM: Model class definitionThe model class correctly uses the dynamically resolved
_ModelBaseClassand maintains proper structure with identifiers, attributes, and typed fields.examples/nautobot-v2_to_infrahub/infrahub/sync_adapter.py (1)
16-16: LGTM: Import reordering.The reordering of
InfraIPAddressin the import list has no functional impact.examples/nautobot-v2_to_infrahub/infrahub/sync_models.py (3)
7-18: Verify that broad exception handling is appropriate.The
except Exception:block catches all exceptions during dynamic base class resolution, which could mask configuration errors, import failures, or bugs inPluginLoader. While the fallback toDiffSyncModelprevents import-time failure, consider whether specific exception types should be caught instead.Since this is auto-generated code, verify that the code generator intentionally uses broad exception handling for robustness, or if it should be narrowed to specific exceptions like
ImportError,AttributeError, orPluginLoadError.
140-149: LGTM: Improved readability.The multi-line formatting of the
_attributestuple improves readability without changing functionality.
26-372: LGTM: Consistent model definitions.All model classes follow a consistent pattern with proper type hints and structure. The expanded model definitions with additional fields and attributes align with the PR's goal of supporting richer schema mappings.
examples/custom_adapter/mockdb/sync_models.py (2)
1-6: LGTM: Import changes are clean.The formatting adjustments and
Anyimport are appropriate for the type annotations used in the model class.
26-34: LGTM: Model class definition is correct.The
InfraDeviceclass properly inherits from the dynamically resolved_ModelBaseClassand has appropriate type annotations.examples/custom_adapter/infrahub/sync_models.py (2)
1-6: LGTM: Import changes are clean.The formatting and
Anyimport are appropriate, consistent with the companion file.
26-34: LGTM: Model class definition is correct.The
InfraDeviceclass properly inherits from the dynamically resolved_ModelBaseClasswith appropriate type annotations.examples/librenms_to_infrahub/genericrestapi/sync_models.py (3)
3-6: LGTM: Import and formatting changes support new models.The addition of
Anytype import and formatting adjustments appropriately support the new model class definitions that follow.
7-19: Dynamic base resolution pattern works well for optional mapping use case.The runtime resolution of
_ModelBaseClassviaPluginLoaderappropriately supports the PR's objective of allowing models without explicit source mappings. The fallback toDiffSyncModelensures import safety.Note: The broad exception handler (line 14) provides robustness but could potentially mask unexpected errors during development.
37-68: LGTM: New model classes are well-structured.The three new model classes (
InfraDevice,IpamIPAddress,LocationSite) are properly structured with consistent type hints, proper identifier/attribute declarations, and support for local metadata vialocal_idandlocal_datafields.examples/librenms_to_infrahub/infrahub/sync_models.py (3)
3-6: LGTM: Import and formatting changes support new models.The addition of
Anytype import and formatting adjustments appropriately support the new model class definitions.
7-19: Dynamic base resolution correctly targets infrahub adapter.The runtime resolution pattern is identical to the genericrestapi sync_models file, with the appropriate adapter spec set to
"infrahub"(line 11). The fallback mechanism ensures import safety.
37-68: LGTM: New model classes are well-structured.The three new model classes (
InfraDevice,IpamIPAddress,LocationSite) are properly structured with consistent type hints and proper metadata field declarations, matching the pattern established in the genericrestapi sync_models file.examples/aci_to_infrahub/infrahub/sync_models.py (2)
1-18: LGTM! Base class resolution is correct.The dynamic base class resolution using
PluginLoaderis properly implemented with a safe fallback toDiffSyncModel. The imports and initialization logic follow the expected pattern.
26-84: LGTM! Generated model classes are structurally correct.The four new model classes (
DcimPhysicalInterface,LocationBuilding,LocationMetro,OrganizationCustomer) follow the correct structure and are consistent with existing patterns. All type hints, identifiers, and attributes are properly defined.The file header confirms this is auto-generated via
infrahub-sync generate, so no manual re-generation step is needed.examples/aci_to_infrahub/aci/sync_models.py (1)
3-3: LGTM! TheAnyimport is correctly added.The
Anytype is properly used for thelocal_datafield type hints across all model classes (lines 38, 50, 62, 73, 83).examples/infrahub_to_peering-manager/peeringmanager/sync_models.py (1)
49-51: No changes required – code is correct.The mutable default values (
= []) in the field declarations are safe. Pydantic automatically deep-copies mutable default values for each model instance, so each instance gets its own independent list rather than sharing a reference. DiffSyncModel is based on Pydantic and uses Python typing to define attributes. The auto-generated code correctly leverages this Pydantic feature and does not introduce the shared-state bug that would occur in plain Python classes.examples/device42_to_infrahub/infrahub/sync_models.py (2)
1-19: LGTM: Runtime loader pattern and imports.The dynamic base class resolution using
PluginLoaderwith a safe fallback toDiffSyncModelis well-implemented. TheAnyimport is appropriately used for the genericlocal_datafield.
47-55: LGTM: OrganizationTenant changes.The addition of the optional
descriptionattribute with aNonedefault follows Python best practices for optional fields.examples/netbox_to_infrahub/netbox/sync_adapter.py (1)
13-13: LGTM — reorder only; no functional impact.Also applies to: 42-43
examples/infrahub_to_peering-manager/infrahub/sync_models.py (1)
49-51: I need to inspect the actual file to determine what type of classes are being used. Let me examine the class definitions in the target file.Based on my analysis of the repository and external documentation, I need to verify whether these classes are Pydantic BaseModel instances, as this critically affects the validity of the mutable default concern.
The review comment assumes traditional Python class behavior where mutable defaults are shared across instances. However, Pydantic automatically creates a deep copy of mutable default values when creating each instance of the model, making the pattern safe in Pydantic models.
Given that
infrahub-syncis a Pydantic-based synchronization project and thesync_models.pyfiles use type annotations with| Nonesyntax, these classes are almost certainly PydanticBaseModelsubclasses. If that's the case, the mutable default patternlist[str] | None = []is safe and requires no changes.No changes recommended — manually verify class type first.
The file
examples/infrahub_to_peering-manager/infrahub/sync_models.py(lines 49-51, 81, 94-96) should be inspected to confirm whether these classes inherit frompydantic.BaseModel. If they do, the mutable defaults are handled correctly and the review comment is incorrect. If they are plain Python classes or dataclasses, the comment is valid.examples/peering-manager_to_infrahub/infrahub/sync_models.py (1)
79-81: Do not manually fix auto-generated files—they will be overwritten on next generation.The target file
examples/peering-manager_to_infrahub/infrahub/sync_models.pyis explicitly marked "AUTO-GENERATED FILE, DO NOT MODIFY" and states "All modifications will be lost the next time you reexecute this command." The same applies to all 24 affected sync_models.py files across examples/.The mutable default issue must be fixed at the source (the generation script/template for
infrahub-sync generate), not in the generated files.However,
infrahub_sync/__init__.py:52is a legitimate non-generated file that does contain an inconsistent mutable default (fields: list[SchemaMappingField] | None = []) within a Pydantic BaseModel, while sibling fields correctly usepydantic.Field(default=None).Likely an incorrect or invalid review comment.
examples/nautobot-v2_to_infrahub/nautobot/sync_models.py (1)
70-70: Auto-generated file—review comment asks to modify code that should not be changed.This file is auto-generated (explicitly marked "DO NOT MODIFY" in the header, line 20-22) and will be regenerated by the
infrahub-sync generatecommand. While the mutable default anti-pattern (= []) is technically present, manually editing auto-generated output is ineffective—any changes will be lost on the next generation.The mutable defaults issue must be fixed at the source: the code generator or template that produces these files, not in the generated output itself.
Likely an incorrect or invalid review comment.
minitriga
left a comment
minitriga
left a comment
There was a problem hiding this comment.
Damn formatting. I found the change at the bottom looks good!
2 participants
Right now, if you are trying to map a static reference to a model not present in the
schema_mappingof the Adapter, it will be unable to find the model, therefore be unable to use it.To bypass that, we want to be able to add schema_mapping without
mappingtowards a source model.Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements