Skip to content

Addition of L4l7 device modules (DCNE-334) #739

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
May 23, 2025
Merged

Conversation

shrsr
Copy link
Collaborator

@shrsr shrsr commented Apr 10, 2025

This PR contains completed aci_l4l7_device and related modules from #186

@shrsr shrsr added the jira-sync Sync this issue to Jira label Apr 10, 2025
@github-actions github-actions bot changed the title Addition of L4l7 device modules Addition of L4l7 device modules (DCNE-334) Apr 10, 2025
aliases: [ concrete_device, concrete_device_name ]
vcenter_name:
description:
- The virtual center name on which the device is hosted in the L4-L7 device cluster.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the virtual center referring to? is this VMware VCenter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely because vCenter is a product that solely belongs to VMware?

@shrsr shrsr requested a review from akinross April 11, 2025 17:18
akinross
akinross previously approved these changes Apr 14, 2025
Copy link
Collaborator

@akinross akinross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

anvitha-jain
anvitha-jain previously approved these changes Apr 14, 2025
Copy link
Collaborator

@anvitha-jain anvitha-jain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

gmicol
gmicol previously approved these changes Apr 18, 2025
Copy link
Collaborator

@gmicol gmicol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines 48 to 53
path_ep:
description:
- The path to the physical interface.
- For single ports, this is the port name, e.g. "eth1/15".
- For Port-channels and vPCs, this is the Interface Policy Group name.
type: str
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move path_ep as an alias and have interface as the main attribute and interface_name and interface_policy_group and interface_policy_group_name as additional aliases.

DOCUMENTATION = r"""
---
module: aci_l4l7_concrete_interface_attach
short_description: Manage L4-L7 Concrete Interface Attach (vns:RsCIfAttN)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
short_description: Manage L4-L7 Concrete Interface Attach (vns:RsCIfAttN)
short_description: Manage L4-L7 Concrete Interface Attachment (vns:RsCIfAttN)

module: aci_l4l7_device
short_description: Manage L4-L7 Devices (vns:LDevVip)
description:
- Manage Layer 4-7 (L4-L7) Devices.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Manage Layer 4-7 (L4-L7) Devices.
- Manage Layer 4 to Layer 7 (L4-L7) Devices.

- The APIC defaults to C(single) when unset during creation.
type: str
choices: [ multi, single ]
dev_type:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

swap attribute and aliases

type: str
choices: [ physical, virtual ]
aliases: [ device_type ]
func_type:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

swap attribute and aliases

dev_type: physical
svc_type: adc
trunking: false
prom_mode: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
prom_mode: true
promiscuous_mode: true

module: aci_l4l7_logical_interface
short_description: Manage L4-L7 Logical Interface (vns:LIf)
description:
- Manage Layer 4-7 (L4-L7) Logical Interfaces.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Manage Layer 4-7 (L4-L7) Logical Interfaces.
- Manage Layer 4 to Layer 7 (L4-L7) Logical Interfaces.

module: aci_l4l7_concrete_interface_attach
short_description: Manage L4-L7 Concrete Interface Attach (vns:RsCIfAttN)
description:
- Manage Layer 4-7 (L4-L7) Concrete Interface Attachment to Logical Interfaces.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Manage Layer 4-7 (L4-L7) Concrete Interface Attachment to Logical Interfaces.
- Manage Layer 4 to Layer 7 (L4-L7) Concrete Interface Attachment to Logical Interfaces.

module: aci_l4l7_concrete_interface
short_description: Manage L4-L7 Concrete Interfaces (vns:CIf)
description:
- Manage Layer 4-7 (L4-L7) Concrete Interfaces.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Manage Layer 4-7 (L4-L7) Concrete Interfaces.
- Manage Layer 4 to Layer 7 (L4-L7) Concrete Interfaces.

module: aci_l4l7_concrete_device
short_description: Manage L4-L7 Concrete Devices (vns:CDev)
description:
- Manage Layer 4-7 (L4-L7) Concrete Devices.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Manage Layer 4-7 (L4-L7) Concrete Devices.
- Manage Layer 4 to Layer 7 (L4-L7) Concrete Devices.

description: More information about the internal APIC class B(vns:CDev)
link: https://developer.cisco.com/docs/apic-mim-ref/
author:
- Tim Cragg (@timcragg)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your name in modules

@@ -0,0 +1,196 @@
# Test code for the ACI modules
# Copyright: (c) 2025, Tim Cragg (@timcragg)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your name in modules tests


DOCUMENTATION = r"""
---
module: aci_l4l7_concrete_interface_attach
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not name the module with full name?

Suggested change
module: aci_l4l7_concrete_interface_attach
module: aci_l4l7_concrete_interface_attachment

@shrsr shrsr requested a review from akinross April 29, 2025 14:58
sajagana
sajagana previously approved these changes Apr 30, 2025
Copy link
Collaborator

@sajagana sajagana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Comment on lines 58 to 59
seealso:
- module: aci_l4l7_device
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we reference the module for the tenant here as well?

Suggested change
seealso:
- module: aci_l4l7_device
seealso:
- module: aci_tenant
- module: aci_l4l7_device

If so it should be applied it to every module where it was missed

Comment on lines 26 to 30
device:
description:
- The name of the logical device (vns:lDevVip) the concrete device is attached to.
type: str
aliases: [ device_name, logical_device_name ]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it not better to call it logical_device and add device as an alias? It would be less confusing since we have logical device and concrete device but this is just a suggestion

Suggested change
device:
description:
- The name of the logical device (vns:lDevVip) the concrete device is attached to.
type: str
aliases: [ device_name, logical_device_name ]
logical_device:
description:
- The name of the logical device (vns:lDevVip) the concrete device is attached to.
type: str
aliases: [ device, device_name, logical_device_name ]

If you do make the change, it should be applied to every module where logical device is referenced

description:
- The name of the L4-L7 device.
type: str
aliases: [ device, device_name, logical_device_name ]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we add an extra alias? logical_device?

Suggested change
aliases: [ device, device_name, logical_device_name ]
aliases: [ device, device_name, logical_device, logical_device_name ]

Comment on lines 26 to 29
device:
description:
- The name of an existing Logical Device.
type: str
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forgot to add the aliases:

Suggested change
device:
description:
- The name of an existing Logical Device.
type: str
device:
description:
- The name of an existing Logical Device.
type: str
aliases: [ logical_device, device_name, logical_device_name ]

Comment on lines +30 to +33
logical_interface:
description:
- The name of an existing Logical Interface.
type: str
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we add a few aliases? like name, logical_interface_name, interface, interface_name, etc...?

Comment on lines 26 to 41
device:
description:
- The name of an existing Logical Device.
type: str
logical_interface:
description:
- The name of an existing Logical Interface.
type: str
concrete_device:
description:
- The name of an existing Concrete Device.
type: str
concrete_interface:
description:
- The name of an existing Concrete Interface.
type: str
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we add the aliases we have defined for these attributes in their respective referenced modules?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The single alias of name in other modules become the main attribute name in this module. Made changes to others!

@shrsr shrsr requested a review from gmicol May 7, 2025 15:55
gmicol
gmicol previously approved these changes May 7, 2025
Copy link
Collaborator

@gmicol gmicol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

- cisco.aci.aci
- cisco.aci.annotation
notes:
- The I(tenant) and I(device) must exist before using this module in your playbook.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this use O for option instead.

Suggested change
- The I(tenant) and I(device) must exist before using this module in your playbook.
- The O(tenant) and O(logical_device) must exist before using this module in your playbook.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment was already made by @gmicol in a related PR. In ansible ACI collection we've been using I() for Input in the other modules. I believe that this usage should be consistent within this collection.

- cisco.aci.aci
- cisco.aci.annotation
notes:
- The I(tenant), I(device) and I(concrete_device) must exist before using this module in your playbook.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- The I(tenant), I(device) and I(concrete_device) must exist before using this module in your playbook.
- The O(tenant), O(logical_device) and O(concrete_device) must exist before using this module in your playbook.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above

- cisco.aci.aci
- cisco.aci.annotation
notes:
- The I(tenant), I(device), I(logical_interface) and I(concrete_device) must exist before using this module in your playbook.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- The I(tenant), I(device), I(logical_interface) and I(concrete_device) must exist before using this module in your playbook.
- The O(tenant), O(logical_device), O(logical_interface) and O(concrete_device) must exist before using this module in your playbook.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above

- cisco.aci.aci
- cisco.aci.annotation
notes:
- The I(tenant) must exist before using this module in your playbook.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- The I(tenant) must exist before using this module in your playbook.
- The O(tenant) must exist before using this module in your playbook.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above

- cisco.aci.aci
- cisco.aci.annotation
notes:
- The I(tenant) and I(device) must exist before using this module in your playbook.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- The I(tenant) and I(device) must exist before using this module in your playbook.
- The O(tenant) and O(logical_device) must exist before using this module in your playbook.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above

@shrsr shrsr requested a review from samiib May 12, 2025 16:07
samiib
samiib previously approved these changes May 14, 2025
Copy link
Collaborator

@samiib samiib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@sajagana sajagana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Collaborator

@akinross akinross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@anvitha-jain anvitha-jain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@gmicol gmicol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@samiib samiib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@lhercot lhercot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lhercot lhercot merged commit 8243460 into CiscoDevNet:master May 23, 2025
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira-sync Sync this issue to Jira
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants