Replaced the ndo modules current value with actual API response value (DCNE-185) #589

sajagana · 2024-12-18T12:45:24Z

Note:

Fixed the below NDO modules:

ndo_synce_interface_policy
ndo_vlan_pool
ndo_dhcp_option_policy
ndo_dhcp_relay_policy
ndo_l3_domain
ndo_physical_domain
ndo_route_map_policy_multicast

plugins/module_utils/mso.py

plugins/modules/ndo_dhcp_option_policy.py

tests/integration/targets/ndo_l3_domain/tasks/main.yml

shrsr

LGTM

gmicol

LGTM

samiib

LGTM

tests/integration/targets/ndo_physical_domain/tasks/main.yml

plugins/modules/ndo_l3_domain.py

plugins/modules/ndo_template.py

… response value and fixed the output value of the ndo_route_map_policy_multicast and ndo_vlan_pool modules

…current portion of the ndo_l3_domain module

gmicol

Sanity's failing on compiling for Python 2.7

akinross

LGTM

gmicol · 2025-02-05T18:36:28Z

plugins/modules/ndo_dhcp_option_policy.py

+def get_dhcp_option_policy(mso_template, uuid=None, name=None, fail_module=False):
+    """
+    Get the DHCP Option Policy by UUID or Name.
+    :param uuid: UUID of the DHCP Option Policy to search for -> Str
+    :param name: Name of the DHCP Option Policy to search for -> Str
+    :param fail_module: When match is not found fail the ansible module -> Bool
+    :return: Dict | None | List[Dict] | List[]: The processed result which could be:
+              When the UUID | Name is existing in the search list -> Dict
+              When the UUID | Name is not existing in the search list -> None
+              When both UUID and Name are None, and the search list is not empty -> List[Dict]
+              When both UUID and Name are None, and the search list is empty -> List[]
+    """


just a quick question about consistency: this get_object function has a description in this module but the other get_object functions doesn't have any description in the following modules. I think they should all have a description or none but up to you to decide

gmicol · 2025-02-05T18:57:23Z

plugins/modules/ndo_l3out_template.py

+    l3outs = mso_template.template.get("l3outTemplate", {}).get("l3outs", [])
    object_description = "L3Out"

    if state in ["query", "absent"] and l3outs == []:
        mso.exit_json()
    elif state == "query" and not (name or uuid):
-        mso.existing = l3outs
+        if l3outs:
+            mso.existing = [insert_l3out_relation_name(l3out, mso_template) for l3out in l3outs]
    elif l3outs and (name or uuid):
-        match = l3out_template_object.get_object_by_key_value_pairs(object_description, l3outs, [KVPair("uuid", uuid) if uuid else KVPair("name", name)])
+        match = mso_template.get_object_by_key_value_pairs(object_description, l3outs, [KVPair("uuid", uuid) if uuid else KVPair("name", name)])
        if match:
-            mso.existing = mso.previous = copy.deepcopy(match.details)
+            mso.existing = mso.previous = copy.deepcopy(insert_l3out_relation_name(match.details, mso_template))


Again a quick question about consistency here: you did not create a get_object function because of the extra conditional statement:

if state in ["query", "absent"] and l3outs == []: mso.exit_json() elif state == "query" and not (name or uuid):

the issue is we should be as consistent as possible across all similar modules. So should we remove these extra conditional statements (they have a slight advantage but nothing that impactful) and add these get_object you were adding to older modules, or should we keep it that way but that means applying the same thing to other modules? I don't know which one is best but I would really appreciate having a discussion on the logic we want to have

gmicol · 2025-02-05T19:02:37Z

plugins/modules/ndo_l3out_template.py

+            )
+
+    if "importRouteMapRef" in l3out_object:
+        if check_mode:
+            l3out_object["importRouteMapName"] = get_template_object_name_by_uuid(mso_template.mso, "routeMap", l3out_object["importRouteMapRef"])
+        else:
+            l3out_object["importRouteMapName"] = get_name_by_key_value(
+                mso_template,
+                "Route Map",
+                l3out_object["importRouteMapRef"],
+                "ref",
+                l3out_relations.get("relations", {}).get("identities", {}).get("routeMapPolicies", []),
+            )
+
+    advanced_route_maps = l3out_object.get("advancedRouteMapRefs")
+    if advanced_route_maps is not None:
+        if "attachedHostRouteRedistRef" in advanced_route_maps:
+            if check_mode:
+                l3out_object["advancedRouteMapRefs"]["attachedHostRouteRedistName"] = get_template_object_name_by_uuid(
+                    mso_template.mso, "routeMap", advanced_route_maps["attachedHostRouteRedistRef"]
+                )
+            else:
+                l3out_object["advancedRouteMapRefs"]["attachedHostRouteRedistName"] = get_name_by_key_value(
+                    mso_template,
+                    "Route Map",
+                    advanced_route_maps["attachedHostRouteRedistRef"],
+                    "ref",
+                    l3out_relations.get("relations", {}).get("identities", {}).get("routeMapPolicies", []),
+                )
+
+        if "connectedRouteRedistRef" in advanced_route_maps:
+            if check_mode:
+                l3out_object["advancedRouteMapRefs"]["connectedRouteRedistName"] = get_template_object_name_by_uuid(
+                    mso_template.mso, "routeMap", advanced_route_maps["connectedRouteRedistRef"]
+                )
+            else:
+                l3out_object["advancedRouteMapRefs"]["connectedRouteRedistName"] = get_name_by_key_value(
+                    mso_template,
+                    "Route Map",
+                    advanced_route_maps["connectedRouteRedistRef"],
+                    "ref",
+                    l3out_relations.get("relations", {}).get("identities", {}).get("routeMapPolicies", []),
+                )
+
+        if "interleakRef" in advanced_route_maps:
+            if check_mode:
+                l3out_object["advancedRouteMapRefs"]["interleakName"] = get_template_object_name_by_uuid(
+                    mso_template.mso, "routeMap", advanced_route_maps["interleakRef"]
+                )
+            else:
+                l3out_object["advancedRouteMapRefs"]["interleakName"] = get_name_by_key_value(
+                    mso_template,
+                    "Route Map",
+                    advanced_route_maps["interleakRef"],
+                    "ref",
+                    l3out_relations.get("relations", {}).get("identities", {}).get("routeMapPolicies", []),
+                )
+
+        if "routeDampeningV4Ref" in advanced_route_maps:
+            if check_mode:
+                l3out_object["advancedRouteMapRefs"]["routeDampeningV4Name"] = get_template_object_name_by_uuid(
+                    mso_template.mso, "routeMap", advanced_route_maps["routeDampeningV4Ref"]
+                )
+            else:
+                l3out_object["advancedRouteMapRefs"]["routeDampeningV4Name"] = get_name_by_key_value(
+                    mso_template,
+                    "Route Map",
+                    advanced_route_maps["routeDampeningV4Ref"],
+                    "ref",
+                    l3out_relations.get("relations", {}).get("identities", {}).get("routeMapPolicies", []),
+                )
+
+        if "routeDampeningV6Ref" in advanced_route_maps:
+            if check_mode:
+                l3out_object["advancedRouteMapRefs"]["routeDampeningV6Name"] = get_template_object_name_by_uuid(
+                    mso_template.mso, "routeMap", advanced_route_maps["routeDampeningV6Ref"]
+                )
+            else:
+                l3out_object["advancedRouteMapRefs"]["routeDampeningV6Name"] = get_name_by_key_value(
+                    mso_template,
+                    "Route Map",
+                    advanced_route_maps["routeDampeningV6Ref"],
+                    "ref",
+                    l3out_relations.get("relations", {}).get("identities", {}).get("routeMapPolicies", []),
+                )
+
+        if "staticRouteRedistRef" in advanced_route_maps:
+            if check_mode:
+                l3out_object["advancedRouteMapRefs"]["staticRouteRedistName"] = get_template_object_name_by_uuid(
+                    mso_template.mso, "routeMap", advanced_route_maps["staticRouteRedistRef"]
+                )
+            else:
+                l3out_object["advancedRouteMapRefs"]["staticRouteRedistName"] = get_name_by_key_value(
+                    mso_template,
+                    "Route Map",
+                    advanced_route_maps["staticRouteRedistRef"],
+                    "ref",
+                    l3out_relations.get("relations", {}).get("identities", {}).get("routeMapPolicies", []),
+                )
+    return l3out_object


I like the idea of inserting name for referenced objects but that's huge amount of API calls in one module, how impactful is it on runtime when you tested?

sajagana requested review from akinross, anvitha-jain, gmicol, lhercot, samiib and shrsr as code owners December 18, 2024 12:45

sajagana linked an issue Dec 18, 2024 that may be closed by this pull request

Update the existing NDO modules to return the actual current value based on the API response (DCNE-185) #538

Open

sajagana force-pushed the 538_ndo_modules_current_value_fix branch from 8c27fc9 to 39d30ea Compare December 19, 2024 08:31

github-actions bot changed the title ~~Replaced the ndo modules current value with actual API response value~~ Replaced the ndo modules current value with actual API response value (DCNE-185) Dec 19, 2024

akinross requested changes Dec 20, 2024

View reviewed changes

plugins/module_utils/mso.py Outdated Show resolved Hide resolved

plugins/modules/ndo_dhcp_option_policy.py Show resolved Hide resolved

plugins/modules/ndo_dhcp_option_policy.py Show resolved Hide resolved

tests/integration/targets/ndo_l3_domain/tasks/main.yml Outdated Show resolved Hide resolved

sajagana force-pushed the 538_ndo_modules_current_value_fix branch from 2240b6b to 5943cf6 Compare December 20, 2024 17:30

sajagana requested a review from akinross December 20, 2024 17:33

shrsr previously approved these changes Jan 7, 2025

View reviewed changes

gmicol previously approved these changes Jan 8, 2025

View reviewed changes

anvitha-jain previously approved these changes Jan 9, 2025

View reviewed changes

samiib previously approved these changes Jan 9, 2025

View reviewed changes

tests/integration/targets/ndo_physical_domain/tasks/main.yml Outdated Show resolved Hide resolved

akinross requested changes Jan 13, 2025

View reviewed changes

plugins/modules/ndo_l3_domain.py Outdated Show resolved Hide resolved

akinross requested changes Jan 13, 2025

View reviewed changes

plugins/modules/ndo_template.py Show resolved Hide resolved

sajagana dismissed stale reviews from samiib, anvitha-jain, gmicol, and shrsr via 076d777 January 29, 2025 15:10

sajagana added 2 commits January 29, 2025 20:47

[major_change] Replaced the ndo modules current value with actual API…

6447bc7

… response value and fixed the output value of the ndo_route_map_policy_multicast and ndo_vlan_pool modules

[ignore] Added vlan pool_uuid and vlan pool name in the previous and …

2eeb493

…current portion of the ndo_l3_domain module

sajagana force-pushed the 538_ndo_modules_current_value_fix branch from 076d777 to 1590fde Compare January 29, 2025 16:36

sajagana requested review from akinross, gmicol, samiib, anvitha-jain and shrsr January 29, 2025 16:45

[ignore] Mapped UUID with name and updated the test files

284f9f2

sajagana force-pushed the 538_ndo_modules_current_value_fix branch from 1590fde to 284f9f2 Compare January 29, 2025 17:05

gmicol requested changes Jan 30, 2025

View reviewed changes

sajagana mentioned this pull request Jan 31, 2025

Adding modules for IGMP interface policy object. (DCNE-128) #577

Open

[ignore] Fixed ndo_interface_setting.py CI issue

fd6ff0b

sajagana requested a review from gmicol January 31, 2025 09:44

akinross approved these changes Feb 3, 2025

View reviewed changes

gmicol requested changes Feb 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replaced the ndo modules current value with actual API response value (DCNE-185) #589

Replaced the ndo modules current value with actual API response value (DCNE-185) #589

sajagana commented Dec 18, 2024

shrsr left a comment

gmicol left a comment

samiib left a comment

gmicol left a comment

akinross left a comment

gmicol Feb 5, 2025

gmicol Feb 5, 2025

gmicol Feb 5, 2025

Replaced the ndo modules current value with actual API response value (DCNE-185) #589

Are you sure you want to change the base?

Replaced the ndo modules current value with actual API response value (DCNE-185) #589

Conversation

sajagana commented Dec 18, 2024

shrsr left a comment

Choose a reason for hiding this comment

gmicol left a comment

Choose a reason for hiding this comment

samiib left a comment

Choose a reason for hiding this comment

gmicol left a comment

Choose a reason for hiding this comment

akinross left a comment

Choose a reason for hiding this comment

gmicol Feb 5, 2025

Choose a reason for hiding this comment

gmicol Feb 5, 2025

Choose a reason for hiding this comment

gmicol Feb 5, 2025

Choose a reason for hiding this comment