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.

Sign up for GitHub

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

[bugfix] Modified module implementations and documentation based off of identified issues (DCNE-160) #679

Merged
merged 10 commits into from
Aug 22, 2024
Merged

[bugfix] Modified module implementations and documentation based off of identified issues (DCNE-160) #679

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2a1744e
[bugfix] Modify interface_configs requirement using required_if depen…
wiebecoding Aug 19, 2024
29c4434
[ignore] Modify provider_tenant requirement using required_if depende…
wiebecoding Aug 19, 2024
02b833b
[ignore] Modify pool_allocation_mode requirement using required_if de…
wiebecoding Aug 19, 2024
ecb4650
[bugfix] Remove duplicate alias name for extepg in aci_l3out_extsubnet
wiebecoding Aug 19, 2024
41e2d13
[ignore] Modify isGuiMessageText impementation in aci_system_banner
wiebecoding Aug 19, 2024
949b74c
[ignore] Adjust failure message for resource policy parameters in aci…
wiebecoding Aug 19, 2024
8fdc184
[ignore] black changes
wiebecoding Aug 19, 2024
00c7381
[ignore] modify assertion error for failed module for aci_vrf_multicast
wiebecoding Aug 20, 2024
463ee66
[ignore] improved isGuiMessageText implementation in aci_system_banner
wiebecoding Aug 20, 2024
3a624ad
[ignore] added test to check that when gui_banner is null that the mo…
wiebecoding Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions plugins/modules/aci_bulk_static_binding_to_epg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,8 +456,8 @@ def main():
argument_spec=argument_spec,
supports_check_mode=True,
required_if=[
["state", "absent", ["ap", "epg", "tenant"]],
["state", "present", ["ap", "epg", "tenant"]],
["state", "absent", ["ap", "epg", "tenant", "interface_configs"]],
["state", "present", ["ap", "epg", "tenant", "interface_configs"]],
],
)

Expand Down
6 changes: 3 additions & 3 deletions plugins/modules/aci_dhcp_relay_provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,9 @@ def main():
["epg_type", "epg", ["anp", "epg"]],
["epg_type", "l2_external", ["l2out_name", "external_epg"]],
["epg_type", "l3_external", ["l3out_name", "external_epg"]],
["epg_type", "l3_external", ["provider_tenant", "tenant"], True],
["epg_type", "l2_external", ["provider_tenant", "tenant"], True],
["epg_type", "epg", ["provider_tenant", "tenant"], True],
["epg_type", "dn", ["dn"]],
],
mutually_exclusive=[
Expand Down Expand Up @@ -385,9 +388,6 @@ def main():
if provider_tenant is None:
provider_tenant = tenant

if epg_type is not None and epg_type != "dn" and provider_tenant is None:
module.fail_json(msg="provider_tenant is required when epg_type is {0}".format(epg_type))

if epg_type == "epg":
tdn = "uni/tn-{0}/ap-{1}/epg-{2}".format(provider_tenant, anp, epg)
elif epg_type == "l2_external":
Expand Down
2 changes: 1 addition & 1 deletion plugins/modules/aci_l3out_extsubnet.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ def main():
argument_spec.update(
tenant=dict(type="str", required=True, aliases=["tenant_name"]),
l3out=dict(type="str", required=True, aliases=["l3out_name"]),
extepg=dict(type="str", required=True, aliases=["extepg_name", "name"]),
extepg=dict(type="str", required=True, aliases=["extepg_name"]),
network=dict(type="str", aliases=["address", "ip"]),
description=dict(type="str", aliases=["descr"]),
subnet_name=dict(type="str", aliases=["name"]),
Expand Down
10 changes: 9 additions & 1 deletion plugins/modules/aci_system_banner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,14 @@ def main():
if state == "present":
regex_url = "^https?:\\/\\/(?:www\\.)?[-a-zA-Z0-9@:%._\\+~#=]{1,256}\\.[a-zA-Z0-9()]{1,6}\\b(?:[-a-zA-Z0-9()@:%_\\+.~#?&\\/=]*)$"

if gui_banner is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you need to update any tests for this? If nothing failed after the change maybe the test is missing some cases that can be added.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't believe that there needs to be any updates to tests for this. The functionality is the same as the removed message - the reason for the change is that if no 'gui_banner' was provided, the module would fail as re.fullmatch couldn't find in 'gui_banner'. This now checks that gui_banner is defined before running the re.fullmatch. Let me know if I am missing something.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the module failed when gui_banner wasn't defined and the tests still passed then that indicates there is a gap in the test cases.
Now that its fixed we should add a test case that checks for gui_banner being undefined and still working.
Just gives us the opportunity to increase our test coverage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it! I have added a test to ensure that the module failure does not occur. Thanks!

if re.fullmatch(regex_url, gui_banner):
is_gui_message_text = "no"
else:
is_gui_message_text = "yes"
else:
is_gui_message_text = "yes"

Copy link
Collaborator

Choose a reason for hiding this comment

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

can be reduced by
if gui_banner is not None and re.fullmatch(regex_url, gui_banner):
is_gui_message_text = "no"
else:
is_gui_message_text = "yes"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it can! Thank you!

aci.payload(
aci_class="aaaPreLoginBanner",
class_config=dict(
Expand All @@ -277,7 +285,7 @@ def main():
bannerMessageSeverity=severity,
guiMessage=gui_banner,
guiTextMessage=gui_alias,
isGuiMessageText="no" if re.fullmatch(regex_url, gui_banner) else "yes",
isGuiMessageText=is_gui_message_text,
message=controller_banner,
showBannerMessage="yes" if application_banner else "no",
switchMessage=switch_banner,
Expand Down
16 changes: 6 additions & 10 deletions plugins/modules/aci_vlan_pool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,8 @@ def main():
argument_spec=argument_spec,
supports_check_mode=True,
required_if=[
["state", "absent", ["pool"]],
["state", "present", ["pool"]],
["state", "absent", ["pool", "pool_allocation_mode"]],
["state", "present", ["pool", "pool_allocation_mode"]],
],
)

Expand All @@ -240,14 +240,10 @@ def main():
state = module.params.get("state")
name_alias = module.params.get("name_alias")

pool_name = pool

# ACI Pool URL requires the allocation mode for vlan and vsan pools (ex: uni/infra/vlanns-[poolname]-static)
if pool is not None:
if pool_allocation_mode is not None:
pool_name = "[{0}]-{1}".format(pool, pool_allocation_mode)
else:
module.fail_json(msg="ACI requires the 'pool_allocation_mode' when 'pool' is provided")
if pool is not None and pool_allocation_mode is not None:
pool_name = "[{0}]-{1}".format(pool, pool_allocation_mode)
else:
pool_name = pool

aci = ACIModule(module)
aci.construct_url(
Expand Down
2 changes: 1 addition & 1 deletion plugins/modules/aci_vrf_multicast.py
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tests have failed with the current code changes.

Copy link
Collaborator Author

@wiebecoding wiebecoding Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Whoops! Looks like I forgot to push the change to the text. Thanks for catching that! I will add it right now.

Original file line number Diff line number Diff line change
Expand Up @@ -506,7 +506,7 @@ def main():
state = module.params.get("state")

if resource_policy and resource_policy.get("reserved_multicast_entries") and resource_policy.get("reserved_route_map") is None:
samiib marked this conversation as resolved.
Show resolved Hide resolved
aci.fail_json(msg="parameters are mutually exclusive: reserved_route_map|reserved_multicast_entries")
aci.fail_json(msg="C(reserved_route_map) must be provided when C(reserved_multicast_entries) are provided")
elif resource_policy and resource_policy.get("reserved_route_map") and not resource_policy.get("reserved_multicast_entries"):
aci.fail_json(msg="C(reserved_multicast_entries) must be provided and greater than 0 when C(reserved_route_map) is provided")
samiib marked this conversation as resolved.
Show resolved Hide resolved

Expand Down
2 changes: 1 addition & 1 deletion tests/integration/targets/aci_dhcp_relay_provider/tasks/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,7 @@
ansible.builtin.assert:
that:
- err_global_epg_relay_provider is failed
- err_global_epg_relay_provider.msg == "provider_tenant is required when epg_type is epg"
- err_global_epg_relay_provider.msg == "epg_type is epg but any of the following are missing{{":"}} provider_tenant, tenant"
- add_global_epg_relay_provider is changed
- add_global_epg_relay_provider.current.0.dhcpRsProv.attributes.annotation == 'orchestrator:ansible'
- add_global_epg_relay_provider.current.0.dhcpRsProv.attributes.dn == "uni/infra/relayp-ansible_global_dhcp_relay/rsprov-[uni/tn-ansible_tenant/ap-ansible_ap/epg-ansible_epg]"
Expand Down
4 changes: 2 additions & 2 deletions tests/integration/targets/aci_vlan_pool/tasks/dynamic.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@
ansible.builtin.assert:
that:
- error_on_missing_required_param is failed
- 'error_on_missing_required_param.msg == "state is present but all of the following are missing: pool"'
- 'error_on_missing_required_param.msg == "state is present but all of the following are missing: pool, pool_allocation_mode"'

- name: Error when together parameter is missing
cisco.aci.aci_vlan_pool:
Expand All @@ -301,4 +301,4 @@
ansible.builtin.assert:
that:
- error_on_missing_together_param is failed
- error_on_missing_together_param.msg == "ACI requires the 'pool_allocation_mode' when 'pool' is provided"
- 'error_on_missing_together_param.msg == "state is present but all of the following are missing: pool_allocation_mode"'
4 changes: 2 additions & 2 deletions tests/integration/targets/aci_vlan_pool/tasks/static.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@
ansible.builtin.assert:
that:
- error_on_missing_required_param is failed
- 'error_on_missing_required_param.msg == "state is present but all of the following are missing: pool"'
- 'error_on_missing_required_param.msg == "state is present but all of the following are missing: pool, pool_allocation_mode"'

- name: Error when together parameter is missing
cisco.aci.aci_vlan_pool:
Expand All @@ -300,4 +300,4 @@
ansible.builtin.assert:
that:
- error_on_missing_together_param is failed
- error_on_missing_together_param.msg == "ACI requires the 'pool_allocation_mode' when 'pool' is provided"
- 'error_on_missing_together_param.msg == "state is present but all of the following are missing: pool_allocation_mode"'
Loading