Skip to content
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

[EPG Static Port] Make VLAN parameter optional when the state is absent (DCNE-300) #608

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

noppanut15
Copy link

@noppanut15 noppanut15 commented Jan 29, 2025

Description

  • Updated the code to make the vlan parameter optional when the state is absent.
  • This change ensures that the vlan parameter is not required when the state is set to absent, preventing potential errors or unnecessary requirements.
  • Added appropriate checks to handle the absence of the vlan parameter gracefully.

New or Affected Module(s):

  • mso_schema_site_anp_epg_staticport

MSO version and MSO Platform

  • Tested with NDO 4.1

APIC version and APIC Platform for Site Level Resources

  • Tested with APIC 5.2

Collection versions

  • cisco.mso 2.9.0

References

@github-actions github-actions bot changed the title [EPG Static Port] Make VLAN parameter optional when the state is absent [EPG Static Port] Make VLAN parameter optional when the state is absent (DCNE-300) Jan 29, 2025
Comment on lines 516 to 540
# ADD STATIC PORT WITHOUT VLAN
- name: Add static port 3 (dpc) to EPG6 of AP2 without VLAN (error)
cisco.mso.mso_schema_site_anp_epg_staticport:
<<: *mso_info
schema: '{{ mso_schema | default("ansible_test") }}'
site: '{{ mso_site | default("ansible_test") }}'
template: Template 1
anp: AP2
epg: EPG6
pod: pod-2
leaf: 102
path: eth1/3
deployment_immediacy: lazy
mode: regular
type: dpc
primary_micro_segment_vlan: 199
state: present
ignore_errors: true
register: nm_add_stat3e6

- name: Verify nm_add_stat3e6
ansible.builtin.assert:
that:
- nm_add_stat3e6 is failed
- nm_add_stat3e6.msg == "state is present or absent but all of the following are missing{{":"}} vlan."
Copy link
Author

Choose a reason for hiding this comment

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

This test case is to ensure that the change won't affect the present state. (vlan is still needed)

Comment on lines 542 to 572
# ADD STATIC PORT WITH VLAN
- name: Add static port 3 (dpc) to EPG6 of AP2 with VLAN (success)
cisco.mso.mso_schema_site_anp_epg_staticport:
<<: *mso_info
schema: '{{ mso_schema | default("ansible_test") }}'
site: '{{ mso_site | default("ansible_test") }}'
template: Template 1
anp: AP2
epg: EPG6
pod: pod-2
leaf: 102
path: eth1/3
vlan: 100
deployment_immediacy: lazy
mode: regular
type: dpc
primary_micro_segment_vlan: 199
state: present
register: nm_add_stat3e6

- name: Verify nm_add_stat3e6
ansible.builtin.assert:
that:
- nm_add_stat3e6 is changed
- nm_add_stat3e6.previous == {}
- nm_add_stat3e6.current.deploymentImmediacy == 'lazy'
- nm_add_stat3e6.current.portEncapVlan == 100
- nm_add_stat3e6.current.microSegVlan == 199
- nm_add_stat3e6.current.path == 'topology/pod-2/paths-102/pathep-[eth1/3]'
- nm_add_stat3e6.current.mode == 'regular'
- nm_add_stat3e6.current.type == 'dpc'
Copy link
Author

@noppanut15 noppanut15 Jan 30, 2025

Choose a reason for hiding this comment

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

This test case is to ensure that the change won't affect the present state. (vlan is still needed). And this static port will be used for deletion in the next test case.

(I checked topology/pod-2/paths-102/pathep-[eth1/3] is not currently in use by the other tests.)

Comment on lines 574 to 515
# REMOVE STATIC PORT WITHOUT VLAN
- name: Remove static port 3 (dpc) from EPG6 of AP2 without VLAN (success)
cisco.mso.mso_schema_site_anp_epg_staticport:
<<: *mso_info
schema: '{{ mso_schema | default("ansible_test") }}'
site: '{{ mso_site | default("ansible_test") }}'
template: Template 1
anp: AP2
epg: EPG6
pod: pod-2
leaf: 102
path: eth1/3
state: absent
register: nm_add_stat3e6

- name: Verify nm_add_stat3e6
ansible.builtin.assert:
that:
- nm_add_stat3e6 is changed
- nm_add_stat3e6.current == {}

Copy link
Author

Choose a reason for hiding this comment

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

Check if we can remove the static port without VLAN

Comment on lines 1254 to 1286
# ADD STATIC PORT WITHOUT VLAN (BULK)
- name: Add static port 3 & 4 (dpc) to epg_bulk of anp_bulk without VLAN (error)
cisco.mso.mso_schema_site_anp_epg_staticport:
<<: *mso_info
schema: '{{ mso_schema | default("ansible_test") }}'
site: '{{ mso_site | default("ansible_test") }}'
template: template_bulk
anp: anp_bulk
epg: epg_bulk
static_ports:
- pod: pod-2
leaf: 102
path: eth1/3
deployment_immediacy: lazy
mode: regular
type: dpc
primary_micro_segment_vlan: 199
- pod: pod-2
leaf: 102
path: eth1/4
deployment_immediacy: lazy
mode: regular
type: dpc
primary_micro_segment_vlan: 199
state: present
ignore_errors: true
register: nm_add_stat3e6

- name: Verify nm_add_stat3e6
ansible.builtin.assert:
that:
- nm_add_stat3e6 is failed
- nm_add_stat3e6.msg == "state is present but all of the following are missing{{":"}} vlan."
Copy link
Author

Choose a reason for hiding this comment

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

[TEST IN BULK] This test case is to ensure that the change won't affect the present state. (vlan is still needed)

Comment on lines 1288 to 1346
# ADD STATIC PORT WITH VLAN (BULK)
- name: Add static port 3 & 4 (dpc) to epg_bulk of anp_bulk with VLAN (success)
cisco.mso.mso_schema_site_anp_epg_staticport:
<<: *mso_info
schema: '{{ mso_schema | default("ansible_test") }}'
site: '{{ mso_site | default("ansible_test") }}'
template: template_bulk
anp: anp_bulk
epg: epg_bulk
static_ports:
- pod: pod-2
leaf: 102
path: eth1/3
vlan: 100
deployment_immediacy: lazy
mode: regular
type: dpc
primary_micro_segment_vlan: 199
- pod: pod-2
leaf: 102
path: eth1/4
vlan: 100
deployment_immediacy: lazy
mode: regular
type: dpc
primary_micro_segment_vlan: 199
state: present
register: nm_add_bulk_stat3e6

- name: Verify nm_add_bulk_stat3e6
ansible.builtin.assert:
that:
- nm_add_bulk_stat3e6 is changed
- nm_add_bulk_stat3e6.previous | length == 10
- nm_add_bulk_stat3e6.previous.0.path == "topology/pod-1/paths-101/pathep-[eth1/1]"
- nm_add_bulk_stat3e6.previous.0.portEncapVlan == 1101
- nm_add_bulk_stat3e6.previous.2.path == "topology/pod-1/paths-101/pathep-[eth1/5]"
- nm_add_bulk_stat3e6.previous.2.portEncapVlan == 1105
- nm_add_bulk_stat3e6.previous.4.path == "topology/pod-1/paths-101/pathep-[eth1/9]"
- nm_add_bulk_stat3e6.previous.4.portEncapVlan == 1109
- nm_add_bulk_stat3e6.previous.5.path == "topology/pod-1/paths-101/pathep-[eth2/1]"
- nm_add_bulk_stat3e6.previous.5.portEncapVlan == 1201
- nm_add_bulk_stat3e6.previous.7.path == "topology/pod-1/paths-101/pathep-[eth2/5]"
- nm_add_bulk_stat3e6.previous.7.portEncapVlan == 1205
- nm_add_bulk_stat3e6.previous.9.path == "topology/pod-1/paths-101/pathep-[eth2/9]"
- nm_add_bulk_stat3e6.previous.9.portEncapVlan == 1209
- nm_add_bulk_stat3e6.current | length == 12
- nm_add_bulk_stat3e6.current.10.deploymentImmediacy == 'lazy'
- nm_add_bulk_stat3e6.current.10.portEncapVlan == 100
- nm_add_bulk_stat3e6.current.10.microSegVlan == 199
- nm_add_bulk_stat3e6.current.10.path == 'topology/pod-2/paths-102/pathep-[eth1/3]'
- nm_add_bulk_stat3e6.current.10.mode == 'regular'
- nm_add_bulk_stat3e6.current.10.type == 'dpc'
- nm_add_bulk_stat3e6.current.11.deploymentImmediacy == 'lazy'
- nm_add_bulk_stat3e6.current.11.portEncapVlan == 100
- nm_add_bulk_stat3e6.current.11.microSegVlan == 199
- nm_add_bulk_stat3e6.current.11.path == 'topology/pod-2/paths-102/pathep-[eth1/4]'
- nm_add_bulk_stat3e6.current.11.mode == 'regular'
- nm_add_bulk_stat3e6.current.11.type == 'dpc'
Copy link
Author

@noppanut15 noppanut15 Jan 30, 2025

Choose a reason for hiding this comment

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

[TEST IN BULK] This test case is to ensure that the change won't affect the present state. (vlan is still needed). And these 2 static ports will be used for deletion in the next test case.

(I checked topology/pod-2/paths-102/pathep-[eth1/3] & topology/pod-2/paths-102/pathep-[eth1/4] are not currently in use by the other tests.)

Comment on lines 1348 to 1384
# REMOVE STATIC PORT WITHOUT VLAN (BULK)
- name: Remove static port 3 & 4 (dpc) from epg_bulk of anp_bulk without VLAN (success)
cisco.mso.mso_schema_site_anp_epg_staticport:
<<: *mso_info
schema: '{{ mso_schema | default("ansible_test") }}'
site: '{{ mso_site | default("ansible_test") }}'
template: template_bulk
anp: anp_bulk
epg: epg_bulk
static_ports:
- pod: pod-2
leaf: 102
path: eth1/3
- pod: pod-2
leaf: 102
path: eth1/4
state: absent
register: nm_rm_bulk_stat3e6

- name: Verify nm_rm_bulk_stat3e6
ansible.builtin.assert:
that:
- nm_rm_bulk_stat3e6 is changed
- nm_rm_bulk_stat3e6.current | length == 10
- nm_add_bulk_stat3e6.current.0.path == "topology/pod-1/paths-101/pathep-[eth1/1]"
- nm_add_bulk_stat3e6.current.0.portEncapVlan == 1101
- nm_add_bulk_stat3e6.current.2.path == "topology/pod-1/paths-101/pathep-[eth1/5]"
- nm_add_bulk_stat3e6.current.2.portEncapVlan == 1105
- nm_add_bulk_stat3e6.current.4.path == "topology/pod-1/paths-101/pathep-[eth1/9]"
- nm_add_bulk_stat3e6.current.4.portEncapVlan == 1109
- nm_add_bulk_stat3e6.current.5.path == "topology/pod-1/paths-101/pathep-[eth2/1]"
- nm_add_bulk_stat3e6.current.5.portEncapVlan == 1201
- nm_add_bulk_stat3e6.current.7.path == "topology/pod-1/paths-101/pathep-[eth2/5]"
- nm_add_bulk_stat3e6.current.7.portEncapVlan == 1205
- nm_add_bulk_stat3e6.current.9.path == "topology/pod-1/paths-101/pathep-[eth2/9]"
- nm_add_bulk_stat3e6.current.9.portEncapVlan == 1209

Copy link
Author

Choose a reason for hiding this comment

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

[TEST IN BULK] Check if we can remove the 2 static ports without VLAN

@noppanut15
Copy link
Author

noppanut15 commented Jan 30, 2025

Hi @akinross,

I've added the integration tests for both single and bulk operations along with the comments above. Feel free to give feedback or let me know if you need any changes. Thanks!

Results

inventory2_ini_—_uc1-ansible-epg-static-binding-2 inventory2_ini_—_uc1-ansible-epg-static-binding

Summary (all tests under the module mso_schema_site_anp_epg_staticport):

  • Tested with my NDO (under a playbook importing the tests using include_tasks.)
image
  • Tested with CI's servers (with ansible-test):
    • sjc19-105-dcn-dmz-nd-ci-01
    • sjc19-105-dcn-dmz-nd-ci-02
    • sjc19-105-dcn-dmz-nd-ci-03
    • sjc19-105-dcn-dmz-nd-ci-04
ansible_collections_ansible_collections_cisco_mso_tests_integration

@noppanut15 noppanut15 requested a review from akinross January 30, 2025 05:00
@@ -512,6 +513,85 @@
that:
- nm_remove_again_stat2e2 is not changed

# ADD STATIC PORT WITHOUT VLAN
Copy link
Collaborator

Choose a reason for hiding this comment

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

this test seems redundant as this behaviour is already tested

- nm_add_stat3e6.msg == "state is present or absent but all of the following are missing{{":"}} vlan."

# ADD STATIC PORT WITH VLAN
- name: Add static port 3 (dpc) to EPG6 of AP2 with VLAN (success)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a clear name of what this static port is used for? We assume success and only (error) on the names usually

- nm_add_stat3e6.msg == "state is present or absent but all of the following are missing{{":"}} vlan."

# ADD STATIC PORT WITH VLAN
- name: Add static port 3 (dpc) to EPG6 of AP2 with VLAN (success)
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you create a new section at the bottom ( when the schemas and required parent objects have not been removed ) that has your tests together with a header like ## WITHOUT VLAN REMOVAL TEST ##

type: dpc
primary_micro_segment_vlan: 199
state: present
register: nm_add_stat3e6
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you use more meaningful names for the registered vars

state: present
register: nm_add_stat3e6

- name: Verify nm_add_stat3e6
Copy link
Collaborator

Choose a reason for hiding this comment

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

assert for add seems redundant as we already test creation with its error and this has not changed in your contribution

@noppanut15 noppanut15 requested a review from akinross January 31, 2025 12:18
@noppanut15 noppanut15 force-pushed the master branch 2 times, most recently from 210e54d to 4db8931 Compare January 31, 2025 12:48
- Updated the code to make the `vlan` parameter optional when the `state` is `absent`.
- This change ensures that the `vlan` parameter is not required when the state is set to `absent`, preventing potential errors or unnecessary requirements.
- Added appropriate checks to handle the absence of the `vlan` parameter gracefully.
@noppanut15
Copy link
Author

I've updated the test and rebased the code to catch up with the upstream.

Also, tested with my NDO.

image

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

@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

@shrsr shrsr 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
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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EPG Static Port] Make VLAN parameter optional when the state is absent (DCNE-300)
6 participants