Add shared module utility support#294
Conversation
| items_to_delete = [existing_item for identifier in diff_identifiers if (existing_item := self.existing.get(identifier)) is not None] | ||
| self._delete_items(items_to_delete) | ||
|
|
||
| def _manage_replace_deletions(self) -> None: |
There was a problem hiding this comment.
Kindly comment on "replace" state if it is okay
sivakasi-cisco
requested review from
akinross,
allenrobel,
anvitha-jain,
gmicol,
lhercot,
mikewiebe,
mtarking,
sajagana,
samiib and
shrsr
as code owners
| @@ -0,0 +1,76 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
what is the difference between data.py and the utils.py, they both seem helper functions is there a reason to add this into a separate file/location?
| if value is None: | ||
| return None | ||
|
|
||
| text = str(value).strip() |
There was a problem hiding this comment.
should we wrap this into a try/except to be sure the str() works?
| @@ -0,0 +1,76 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
please add unit tests for all the helper functions
|
|
||
| text = str(value).strip() | ||
| if not text: | ||
| return None |
There was a problem hiding this comment.
why return None for an empty text (assuming here text is always a string) and not an empty string to preserve it's type?
| return {} | ||
|
|
||
|
|
||
| def loads_maybe_json(value: Any) -> Any: |
There was a problem hiding this comment.
Why name the function loads_maybe_json when it could be json or python literal string? I think we should make this more naming more specific, the function more generic or the function more specific.
Personally i would make the function more generic and extensible by calling it parse_value, where the function takes a value and a list of parsers like json.loads and ast.literal_eval, this way if you want to extend parsing capabilities in the future you should be able to leverage the function.
| try: | ||
| return json.loads(text) | ||
| except Exception: | ||
| try: |
There was a problem hiding this comment.
nested try/except are typically a code smell, think we should use a loop alternative
| return None | ||
|
|
||
|
|
||
| def coerce_dict_list(data: Any, list_keys: tuple[str, ...] = ("DATA", "data", "items")) -> list[dict[str, Any]]: |
There was a problem hiding this comment.
should this function also support deeper levels of nesting?
can you document the restrictions of this function more clearly?
| return [] | ||
|
|
||
|
|
||
| def copy_dict_items(items: Any) -> list[dict[str, Any]]: |
There was a problem hiding this comment.
we should a stricter type hinting on items, probably something like Optional[Iterable[Any]] or Iterable[Mapping[str, Any] | BaseModel]
| # Initialize collections | ||
| try: | ||
| response_data = self.model_orchestrator.query_all() | ||
| config_data = self.module.params.get("config", []) |
There was a problem hiding this comment.
dict.get(key, default) only returns default when the key is missing. If config is explicitly present with value None, you get None back — not []. is this intended else change to below
| config_data = self.module.params.get("config", []) | |
| config_data = self.module.params.get("config") or [] |
2 participants
Problem Description
This PR separates out a few shared framework changes that are needed before modules like
nd_manage_vrf_litecan cleanly follow the generic ND 4.x module architecture - #281.Some ND 4.x resources are simple: one playbook item maps directly to one controller object. In those cases, the existing generic state machine can compare “what the user wants” with “what exists on the controller” and decide create, update, delete, or no-op.
But some resources are nested. VRF Lite is a good example: the user gives VRF-level config, but the actual work happens under smaller child objects like VRF attachments and VRF Lite links. Without small shared framework improvements, each nested module has to write its own custom comparison logic.
This PR adds the common support needed for that style of module.
Why These Common Changes Are Needed
nd_state_machine.pynow allows a module to normalize or flatten playbook config after reading current controller state. This lets nested modules convert user input into the smaller objects that the generic state machine can compare.nd_state_machine.pyalso lets modules handle scoped delete behavior forreplacedstate. This is useful when only child objects under a specific parent should be removed.nd_state_machine.pynow tracks deleted items insent, so later steps like save/deploy can know what was actually changed.models/base.pyandutils.pyimprove diff comparison so controller-returned extra fields do not cause false “changed” results when the user did not specify those fields.common/data.pyadds small reusable helpers for common controller response handling, JSON parsing, dict-list cleanup, object copying, and safe integer conversion.