Addition of a new module for enhanced_lag_policy (DCNE-422) #768

DevSinha13 · 2025-05-29T20:42:04Z

No description provided.

akinross · 2025-06-02T08:55:57Z

plugins/modules/aci_vmm_enhanced_lag_policy.py

+    state: query
+  register: query_all_result
+
+- name: Ensure idempotency when creating an Enhanced LACP Policy


dont think we include this type of example in other modules

akinross · 2025-06-02T08:56:03Z

plugins/modules/aci_vmm_enhanced_lag_policy.py

+    vm_provider: vmware
+    state: absent
+
+- name: Simulate deletion of an Enhanced LACP Policy (Check Mode)


dont think we include this type of example in other modules

Noted, I have deleted that example. Thank you for the clarification.

akinross · 2025-06-02T08:57:58Z

plugins/modules/aci_vmm_enhanced_lag_policy.py

+    argument_spec.update(
+        domain=dict(type="str", aliases=["domain_name", "domain_profile"]),
+        state=dict(type="str", default="present", choices=["absent", "present", "query"]),
+        vm_provider=dict(type="str", choices=list(VM_PROVIDER_MAPPING.keys())),


don't think the keys() is needed and list function can be called directly on the MAP

Suggested change

vm_provider=dict(type="str", choices=list(VM_PROVIDER_MAPPING.keys())),

vm_provider=dict(type="str", choices=list(VM_PROVIDER_MAPPING)),

Fixed, removed the .keys() function and maintained functionality across all tests. Thanks for the feedback.

akinross · 2025-06-02T08:59:33Z

plugins/modules/aci_vmm_enhanced_lag_policy.py

+    aci_owner_spec,
+)
+
+VM_PROVIDER_MAPPING = dict(


You can import and use the VM_PROVIDER_MAPPING defined in module_utils/constants.py

plugins/modules/aci_vmm_enhanced_lag_policy.py

akinross · 2025-06-02T09:07:33Z

plugins/modules/aci_vmm_enhanced_lag_policy.py

+    number_uplinks: 4
+    state: present
+
+- name: Simulate creation of an Enhanced LACP Policy (Check Mode)


dont think we include this type of example in other modules

Not sure I understand the thumbs up here? Has this been decided to include?

akinross · 2025-06-02T09:09:33Z

plugins/modules/aci_vmm_enhanced_lag_policy.py

+description:
+- Manage Enhanced LACP Policy (lacpEnhancedLagPol) for VMM domains on Cisco ACI fabrics.
+- The Enhanced LACP Policy allows you to configure advanced Link Aggregation Control Protocol (LACP) settings for virtual switches in VMM domains.
+- This policy is a child of the C(vmmVSwitchPolicyCont) class.


is children something we decided to add to the descriptions?

Not sure I understand the thumbs up here? Has this been decided?

akinross · 2025-06-02T09:10:53Z

plugins/modules/aci_vmm_enhanced_lag_policy.py

+DOCUMENTATION = r"""
+---
+module: aci_vmm_enhanced_lag_policy
+short_description: Manage Enhanced LACP Policy for Virtual Machine Manager (VMM) in Cisco ACI


In other modules I think we include the classname to the short description (lacp:EnhancedLagPol) at the end. If not mistaken this was done for search-ability of classes in documentation

akinross · 2025-06-02T09:12:04Z

plugins/modules/aci_vmm_enhanced_lag_policy.py

+module: aci_vmm_enhanced_lag_policy
+short_description: Manage Enhanced LACP Policy for Virtual Machine Manager (VMM) in Cisco ACI
+description:
+- Manage Enhanced LACP Policy (lacpEnhancedLagPol) for VMM domains on Cisco ACI fabrics.


don't think we put class names in the descriptions

plugins/modules/aci_vmm_enhanced_lag_policy.py

…IDER_MAPPING

…and replaced names with vendor in target_filter

plugins/modules/aci_vmm_enhanced_lag_policy.py

Co-authored-by: Akini Ross <akinross@cisco.com>

… can use this module in a playbook

akinross · 2025-06-10T09:19:27Z

plugins/modules/aci_vmm_enhanced_lag_policy.py

+description:
+- Manage Enhanced LACP Policy (lacpEnhancedLagPol) for VMM domains on Cisco ACI fabrics.
+- The Enhanced LACP Policy allows you to configure advanced Link Aggregation Control Protocol (LACP) settings for virtual switches in VMM domains.
+- This policy is a child of the C(vmmVSwitchPolicyCont) class.


Not sure I understand the thumbs up here? Has this been decided?

plugins/modules/aci_vmm_enhanced_lag_policy.py

akinross · 2025-06-10T09:24:19Z

plugins/modules/aci_vmm_enhanced_lag_policy.py

+    number_uplinks: 4
+    state: present
+
+- name: Simulate creation of an Enhanced LACP Policy (Check Mode)


Not sure I understand the thumbs up here? Has this been decided to include?

akinross · 2025-06-10T09:27:38Z

plugins/modules/aci_vmm_enhanced_lag_policy.py

+            aci_class="vmmVSwitchPolicyCont",
+            aci_rn="vswitchpolcont",
+            module_object="vswitchpolcont",
+            target_filter={"name": "vswitchpolcont"},


I would start by testing the behaviour with and without for both module_object and target_filter.

The target_filter is referencing a property now that exist but is not configurable according to the model, so is it always vswitchpolcont?

akinross · 2025-06-11T12:39:03Z

plugins/modules/aci_vmm_enhanced_lag_policy.py

+- The I(vmm_domain) and I(vSwitch_policy) must exist before using this module in a playbook.
+- The modules M(cisco.aci.aci_domain) and M(cisco.aci.aci_vmm_vswitch_policy) can be used for this.
+seealso:
+- module: cisco.aci.aci_domain


Suggested change

- module: cisco.aci.aci_domain

- module: cisco.aci.aci_domain

- module: cisco.aci. aci_vmm_vswitch_policy

Sorry about that, I have made the fix

akinross · 2025-06-11T12:57:16Z

plugins/modules/aci_vmm_enhanced_lag_policy.py

+            aci_class="vmmVSwitchPolicyCont",
+            aci_rn="vswitchpolcont",
+            module_object="vswitchpolcont",
+            target_filter={"name": "vswitchpolcont"},


had a quick look and from what I read in code if module_object is defined that should be enough, target_filter is not used at all in the logic as far as I can tell so can be excluded, since it only uses the filter of the object defined in subclass_3. See _construct_url_4) function in aci.py

akinross

LGTM

…ocumentation

shrsr

LGTM

Dev-Sinha13 added 4 commits May 20, 2025 15:14

[minor_change] Added support for description to aci_syslog_group module.

4c70cc3

[ignore] Updated Test file for aci_syslog_group

1551ff6

Missing explaination but tests are passing now.

29c003d

Added black formatting and documentation.

10d5480

DevSinha13 requested review from akinross, anvitha-jain, gmicol, lhercot, sajagana, samiib and shrsr as code owners May 29, 2025 20:42

DevSinha13 linked an issue May 29, 2025 that may be closed by this pull request

Create a new module for enhanced lag policy for the VMM domain (aci_enhanced_lag_policy) (DCNE-422) #759

Open

[ignore] Fixed documentation and fixed sanity issues

6cd9f92

github-actions bot changed the title ~~Addition of a new module for enhanced_lag_policy~~ Addition of a new module for enhanced_lag_policy (DCNE-422) May 30, 2025

akinross requested changes Jun 2, 2025

View reviewed changes

DevSinha13 requested a review from akinross June 2, 2025 23:48

Dev-Sinha13 added 2 commits June 4, 2025 10:28

[ignore] Fixed documentation and examples as well as imported VM_PROV…

61a2709

…IDER_MAPPING

[minor change] Removed the .keys() call from the VM_PROVIDER_MAPPING …

187af50

…and replaced names with vendor in target_filter

akinross requested changes Jun 4, 2025

View reviewed changes

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

DevSinha13 and others added 2 commits June 4, 2025 11:05

Update plugins/modules/aci_vmm_enhanced_lag_policy.py

146ad7f

Co-authored-by: Akini Ross <akinross@cisco.com>

[ignore] fixed sanity issues for default values

e9a8cce

gmicol requested a review from akinross June 5, 2025 18:27

Dev-Sinha13 added 2 commits June 9, 2025 10:51

[ignore] Added a note for the prior configuration required before you…

b2a58b8

… can use this module in a playbook

[ignore] Fixed sanity issues

992c92f

akinross requested changes Jun 10, 2025

View reviewed changes

Dev-Sinha13 added 2 commits June 10, 2025 09:44

[ignore] fixed documentation issues with examples and description

493411e

[ignore] fixed notes for prior configuration

f918f35

DevSinha13 requested a review from akinross June 10, 2025 22:37

akinross requested changes Jun 11, 2025

View reviewed changes

DevSinha13 requested a review from akinross June 11, 2025 18:58

akinross previously approved these changes Jun 12, 2025

View reviewed changes

DevSinha13 dismissed akinross’s stale review via 3ab2f57 June 12, 2025 18:27

[minor change] removed target filter for vmmVSwitchPolicy and fixed d…

bc2bc36

…ocumentation

DevSinha13 force-pushed the feature/aci_enhanced_lag_policy branch from 3ab2f57 to bc2bc36 Compare June 12, 2025 18:40

shrsr requested a review from akinross June 12, 2025 21:44

shrsr approved these changes Jun 12, 2025

View reviewed changes

	vm_provider=dict(type="str", choices=list(VM_PROVIDER_MAPPING.keys())),
	vm_provider=dict(type="str", choices=list(VM_PROVIDER_MAPPING)),

	- module: cisco.aci.aci_domain
	- module: cisco.aci.aci_domain
	- module: cisco.aci. aci_vmm_vswitch_policy

Addition of a new module for enhanced_lag_policy (DCNE-422) #768

Are you sure you want to change the base?

Addition of a new module for enhanced_lag_policy (DCNE-422) #768

Uh oh!

Conversation

DevSinha13 commented May 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akinross left a comment

Choose a reason for hiding this comment

Uh oh!

shrsr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!