Aci adapter feature #100
Aci adapter feature #100
Conversation
WalkthroughAdds a Cisco ACI integration and examples: new AciApiClient (authentication, token refresh, retries, error handling), AciAdapter and AciModel (device mapping, schema-driven model loading, Jinja filter Pre-merge checks and finishing touches✅ Passed checks (5 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: 6
🧹 Nitpick comments (4)
docs/docs/adapters/local-adapters.mdx (1)
142-160: Clarify persistence of transformed fields not listed infields.If a transform sets a field (e.g.,
status) that isn’t declared underfieldsfor the model, it typically won’t be included in the payload passed to the DiffSync model constructor. Consider adding a note here that such fields should also be listed infieldsand supported by the target model’s attributes to be persisted.examples/aci_to_infrahub/config.yml (1)
99-116: Validate transformed fields align with model attributes.You set
DcimPhysicalInterface.statusvia transform, but the example model’s_attributesonly include("description",). Ifstatusis meant to persist, ensure it’s declared in the model and included infields.infrahub_sync/adapters/aci.py (2)
171-183: Include response body snippet in HTTP error for diagnostics.Adding a truncated
response.textgreatly aids troubleshooting without exposing excessive data.Apply this diff:
- msg = ( - f"There was an HTTP error while performing operation on {self.base_url}: " - f"Error: {response.status_code}, Reason: {response.reason}" - ) + body = "" + try: + body = response.text[:500] + except Exception: + body = "<unavailable>" + msg = ( + f"There was an HTTP error while performing operation on {self.base_url}: " + f"Error: {response.status_code}, Reason: {response.reason}, Body: {body}" + )
119-127: Timeout is good; consider per-call overrides if needed.30s global timeout is sensible. If some ACI endpoints are slower, expose a per-call timeout or a configurable setting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
docs/docs/adapters/aci.mdx(1 hunks)docs/docs/adapters/local-adapters.mdx(1 hunks)docs/docs/reference/cli.mdx(3 hunks)docs/sidebars.ts(1 hunks)examples/aci_to_infrahub/aci/__init__.py(1 hunks)examples/aci_to_infrahub/aci/sync_adapter.py(1 hunks)examples/aci_to_infrahub/aci/sync_models.py(1 hunks)examples/aci_to_infrahub/config.yml(1 hunks)examples/aci_to_infrahub/infrahub/__init__.py(1 hunks)examples/aci_to_infrahub/infrahub/sync_adapter.py(1 hunks)examples/aci_to_infrahub/infrahub/sync_models.py(1 hunks)infrahub_sync/__init__.py(1 hunks)infrahub_sync/adapters/aci.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
examples/aci_to_infrahub/infrahub/sync_adapter.py (2)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)examples/aci_to_infrahub/infrahub/sync_models.py (5)
DcimPhysicalDevice(23-35)DcimPhysicalInterface(37-46)LocationBuilding(48-56)LocationGeneric(68-76)OrganizationCustomer(58-66)
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/aci_to_infrahub/infrahub/sync_models.py (1)
infrahub_sync/plugin_loader.py (2)
PluginLoader(46-481)from_env_and_args(68-104)
examples/aci_to_infrahub/aci/sync_adapter.py (2)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)examples/aci_to_infrahub/aci/sync_models.py (5)
DcimPhysicalDevice(23-35)DcimPhysicalInterface(37-46)LocationBuilding(48-56)LocationGeneric(68-76)OrganizationCustomer(58-66)
infrahub_sync/adapters/aci.py (2)
infrahub_sync/__init__.py (9)
DiffSyncMixin(130-142)DiffSyncModelMixin(145-261)SchemaMappingModel(46-52)SyncAdapter(55-58)SyncConfig(66-91)model_loader(141-142)filter_records(218-229)transform_records(232-243)is_list(255-261)infrahub_sync/adapters/utils.py (1)
get_value(42-57)
infrahub_sync/__init__.py (1)
infrahub_sync/adapters/aci.py (1)
_add_custom_filters(436-451)
🔇 Additional comments (9)
docs/docs/reference/cli.mdx (1)
39-39: LGTM!The documentation for the new
--adapter-pathCLI option is clear and consistent across all three commands (diff, generate, sync).Also applies to: 58-58, 94-94
infrahub_sync/__init__.py (1)
188-191: LGTM!The extensibility hook is well-designed. It allows subclasses to register custom Jinja filters by implementing
_add_custom_filters, enabling adapter-specific transformations without modifying the base class. Thehasattrcheck ensures backward compatibility.examples/aci_to_infrahub/aci/sync_models.py (1)
23-76: LGTM!The model class declarations are well-structured with proper metadata (
_modelname,_identifiers,_attributes) and appropriate type annotations.docs/sidebars.ts (1)
28-28: LGTM!The new ACI adapter entry is correctly added to the adapters section in the sidebar configuration.
docs/docs/adapters/aci.mdx (1)
1-170: LGTM!The ACI adapter documentation is comprehensive and well-structured. It covers configuration, schema mapping examples, custom Jinja filters, model generation, and troubleshooting, providing users with clear guidance for using the adapter.
examples/aci_to_infrahub/infrahub/sync_adapter.py (1)
23-28: LGTM!The
InfrahubSyncadapter class correctly exposes the model mappings as class attributes.examples/aci_to_infrahub/aci/sync_adapter.py (1)
15-23: LGTM for dynamic adapter resolution.Dynamic resolution via
PluginLoader().resolve("aci")and public model bindings are consistent with the loader’s contract.examples/aci_to_infrahub/config.yml (1)
19-24: Confirm load order includes all mapped models.
LocationGenericis defined below but not listed inorder. If your loader usesorderto drive loading, add it; otherwise, ignore.examples/aci_to_infrahub/infrahub/sync_models.py (1)
5-17: Robust base resolution with safe fallback.Using
PluginLoader.from_env_and_args()and falling back toDiffSyncModelavoids import-time failures. Looks good.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
docs/docs/adapters/aci.mdx (1)
4-4: Remove unused MDX import.The imported component isn't used and may trigger MDX/TS lints in the docs build.
-import ReferenceLink from "../../src/components/Card";examples/aci_to_infrahub/aci/sync_models.py (1)
3-3: Remove unused import.List isn’t used.
-from typing import Any, List +from typing import Anyexamples/aci_to_infrahub/infrahub/sync_models.py (1)
3-3: Remove unused import.List isn’t used.
-from typing import Any, List +from typing import Anyexamples/aci_to_infrahub/schema.yml (1)
23-27: Drop redundant optional on an absent field.description is marked state: absent; optional is redundant and confusing.
- - name: description - state: absent - kind: Text - optional: true + - name: description + state: absent + kind: Text order_weight: 2000infrahub_sync/generator/templates/diffsync_models.j2 (1)
3-3: Remove unused import from template.List isn’t referenced in generated files.
-from typing import Any, List +from typing import Anyinfrahub_sync/adapters/aci.py (4)
219-219: Clarify or remove vague comment.The comment "Default endpoint, change if necessary" doesn't provide actionable guidance. Either document when/why to change it or remove the comment.
266-266: Avoid hardcoding adapter name in source check.The source adapter check uses a hardcoded string
"aci"which is brittle. Consider comparing againstself.config.source.name.lower()withself.type.lower()or using the adapter's name attribute for a more maintainable comparison.Apply this diff:
- is_source_adapter = self.config.source.name.lower() == "aci" + is_source_adapter = self.config.source.name.lower() == self.type.lower()
306-314: Clarify or fix confusing condition.The condition on line 309
if not nodes and all_nodes_for_reference:is confusing. Ifnot nodesis True (the list is empty), checkingall_nodes_for_referencedoesn't provide additional information sincenodeswas derived fromall_nodes_for_reference.Consider whether this should be:
if all_nodes_for_reference is None:(checking if store lookup failed)- Or simply
if not nodes:(checking if no nodes exist for the reference)If the intent is to distinguish between "store lookup failed" vs "no nodes found", use:
- if not nodes and all_nodes_for_reference: + if all_nodes_for_reference is None:Otherwise, simplify to:
- if not nodes and all_nodes_for_reference: + if not nodes:
452-464: Address TODO comments for create and update methods.The
createandupdatemethods have TODO comments indicating they need implementation. These methods are called during DiffSync reconciliation operations.If these methods are intentionally deferred for future implementation, consider:
- Adding more detailed comments explaining the deferral reason and timeline
- Raising
NotImplementedErrorinstead of callingsuper()to make the incomplete state explicitOtherwise, if write operations are expected to work in this PR, these methods need implementation.
Do you want me to generate implementation stubs or open a tracking issue for this work?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/docs/adapters/aci.mdx(1 hunks)examples/aci_to_infrahub/aci/sync_models.py(1 hunks)examples/aci_to_infrahub/config.yml(1 hunks)examples/aci_to_infrahub/infrahub/sync_models.py(1 hunks)examples/aci_to_infrahub/schema.yml(1 hunks)infrahub_sync/adapters/aci.py(1 hunks)infrahub_sync/generator/templates/diffsync_models.j2(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/aci_to_infrahub/config.yml
🧰 Additional context used
🧬 Code graph analysis (3)
examples/aci_to_infrahub/aci/sync_models.py (2)
infrahub_sync/plugin_loader.py (2)
PluginLoader(46-481)from_env_and_args(68-104)examples/aci_to_infrahub/infrahub/sync_models.py (5)
DcimPhysicalDevice(23-35)DcimPhysicalInterface(37-46)LocationBuilding(48-56)OrganizationCustomer(58-66)LocationGeneric(68-76)
examples/aci_to_infrahub/infrahub/sync_models.py (2)
infrahub_sync/plugin_loader.py (2)
PluginLoader(46-481)from_env_and_args(68-104)examples/aci_to_infrahub/aci/sync_models.py (5)
DcimPhysicalDevice(23-35)DcimPhysicalInterface(37-46)LocationBuilding(48-56)OrganizationCustomer(58-66)LocationGeneric(68-76)
infrahub_sync/adapters/aci.py (2)
infrahub_sync/__init__.py (9)
DiffSyncMixin(130-142)DiffSyncModelMixin(145-261)SchemaMappingModel(46-52)SyncAdapter(55-58)SyncConfig(66-91)model_loader(141-142)filter_records(218-229)transform_records(232-243)is_list(255-261)infrahub_sync/adapters/utils.py (1)
get_value(42-57)
🔇 Additional comments (3)
examples/aci_to_infrahub/aci/sync_models.py (1)
7-12: Good: loader honors env/config now.Using PluginLoader.from_env_and_args() (no forced empty paths) addresses earlier path‑resolution concerns.
examples/aci_to_infrahub/infrahub/sync_models.py (1)
7-12: Good: loader honors env/config now.Same safe pattern here; resolves base via PluginLoader with a sane fallback.
infrahub_sync/adapters/aci.py (1)
84-85: Previous review issues have been addressed.Great work fixing the issues from the previous review:
- TLS warnings are now only suppressed when
verify=Falsefor this specific client instance (lines 84-85)- Retry strategy is now restricted to safe HTTP methods (line 93)
- Device mapping is set immediately after building it, ensuring Jinja filters have access before transforms run (line 207)
Also applies to: 93-93, 207-207
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
examples/aci_to_infrahub/infrahub/sync_adapter.py (1)
13-15: Usefrom_env_and_args()and add exception handling for consistency.The
PluginLoaderis instantiated without arguments, ignoring adapter paths from configuration or environment variables. This issue was already flagged in a previous review.Apply this diff to align with the pattern used in sync_models.py:
# Load adapter class dynamically at runtime - -_AdapterBaseClass = PluginLoader().resolve("infrahub") +try: + _loader = PluginLoader.from_env_and_args() + _AdapterBaseClass = _loader.resolve("infrahub") +except Exception: + # Fallback to Adapter base class if resolution fails + from infrahub_sync import Adapter as _AdapterBaseClass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/aci_to_infrahub/aci/sync_adapter.py(1 hunks)examples/aci_to_infrahub/aci/sync_models.py(1 hunks)examples/aci_to_infrahub/config.yml(1 hunks)examples/aci_to_infrahub/infrahub/sync_adapter.py(1 hunks)examples/aci_to_infrahub/infrahub/sync_models.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
examples/aci_to_infrahub/aci/sync_adapter.py (2)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)examples/aci_to_infrahub/aci/sync_models.py (5)
DcimPhysicalDevice(25-37)DcimPhysicalInterface(39-48)LocationBuilding(50-59)LocationMetro(61-69)OrganizationCustomer(71-79)
examples/aci_to_infrahub/infrahub/sync_adapter.py (2)
infrahub_sync/plugin_loader.py (1)
PluginLoader(46-481)examples/aci_to_infrahub/infrahub/sync_models.py (5)
DcimPhysicalDevice(25-37)DcimPhysicalInterface(39-48)LocationBuilding(50-59)LocationMetro(61-69)OrganizationCustomer(71-79)
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/aci_to_infrahub/aci/sync_models.py (1)
infrahub_sync/plugin_loader.py (2)
PluginLoader(46-481)from_env_and_args(68-104)
🔇 Additional comments (6)
examples/aci_to_infrahub/infrahub/sync_models.py (2)
5-18: LGTM! Well-designed dynamic resolution with safe fallback.The use of
PluginLoader.from_env_and_args()correctly honors adapter path configuration from environment variables and arguments. The exception handling with fallback toDiffSyncModelensures graceful degradation if the "infrahub" adapter cannot be resolved.
25-79: LGTM! Models follow Pydantic v2 conventions.All model classes correctly declare explicit defaults for Optional fields (e.g.,
serial: str | None = None), which is required for Pydantic v2 compatibility. The model structure is consistent across all five classes.examples/aci_to_infrahub/config.yml (2)
8-12: LGTM! Credentials properly externalized.The use of environment variables for sensitive credentials (
${CISCO_APIC_URL},${CISCO_APIC_USERNAME},${CISCO_APIC_PASSWORD}) follows security best practices. This addresses the previous concern about plaintext credentials.
114-114:aci_device_namefilter is properly registered: it’s defined inAciAdapter._add_custom_filters(infrahub_sync/adapters/aci.py) and added to the Jinja environment viaapply_transform.examples/aci_to_infrahub/aci/sync_models.py (2)
5-18: LGTM! Correct dynamic resolution pattern.The use of
PluginLoader.from_env_and_args()with exception handling follows the correct pattern and is consistent with the infrahub sync_models.py implementation. The fallback toDiffSyncModelensures graceful degradation.
25-79: LGTM! Pydantic v2-compatible model definitions.All model classes correctly declare explicit defaults for Optional fields (e.g.,
serial: str | None = None,customer_id: str | None = None), which satisfies Pydantic v2 requirements. The model structure is consistent and well-defined.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/README.md(1 hunks)docs/docs/adapters/aci.mdx(1 hunks)docs/docs/adapters/local-adapters.mdx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/docs/adapters/local-adapters.mdx
🧰 Additional context used
🪛 LanguageTool
docs/docs/adapters/aci.mdx
[grammar] ~50-~50: Use a hyphen to join words.
Context: ...ogin timestamps in UTC to avoid timezone related issues and ensure correct token ...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (6)
docs/README.md (1)
11-11: Style consistency improvement.Lowercasing the section heading aligns with the existing documentation convention (Installation, Build, Deployment, etc.). No functional impact.
docs/docs/adapters/aci.mdx (5)
102-136: ACI-specific Jinja filter documentation is comprehensive.The
aci_device_namefilter documentation effectively explains the purpose, usage, common use case, and implementation details. The step-by-step breakdown of the transform logic (extract node ID, remove prefix, resolve to device name) is clear and helpful for users. Troubleshooting guidance for filter issues is well-positioned.
145-172: Troubleshooting section is thorough and practical.The troubleshooting section appropriately covers the main pain points (authentication/connectivity, device reference resolution, Jinja filter issues, general debugging) with specific error messages, diagnostic steps, and remediation guidance. The debugging tip about enabling DEBUG logging and checking build logs is helpful.
26-37: Documentation parameters are accurate and match implementation.All configuration parameters in the documentation match the AciApiClient implementation:
- url: Extracted from settings (line 211) and combined with api_endpoint to form base_url ✓
- username: Passed directly to AciApiClient (line 212) ✓
- password: Passed directly to AciApiClient (line 213) ✓
- api_endpoint: Retrieved from settings with default "api" (line 219) ✓
- verify: Accepts both boolean and string values ("false", "0") as documented (line 218) ✓
Note: The implementation also accepts "no" for the verify parameter, though this is not mentioned in the documentation example.
87-96: Jinja filter implementation verified as accurate.The
aci_device_namefilter is correctly implemented ininfrahub_sync/adapters/aci.py(lines 438-450). The implementation usescls._device_mapping.get(str(node_id), node_id)to resolve node IDs to device names and falls back to the original node ID if not found, which matches the documented behavior. The example DN parsing logic—extracting index [2] from a split DN, then removing the "node-" prefix—is accurate for typical ACI Distinguished Name structures.
39-44: All environment variable names and verify parameter normalization behavior are correctly implemented.The documentation accurately reflects the implementation:
- All four CISCO_APIC_* variables (URL, USERNAME, PASSWORD, VERIFY) are correctly retrieved via
os.environ.get()ininfrahub_sync/adapters/aci.py(lines 211-215)- String normalization for the verify parameter is properly implemented (line 218): strings matching "0", "false", or "no" (case-insensitive) are normalized to False; all others normalize to True
BeArchiTek
left a comment
BeArchiTek
left a comment
There was a problem hiding this comment.
Thanks @t0m3kz ! LGTM
2 participants
Close #61
Summary by CodeRabbit
New Features
Documentation
Examples
Chores
Schema