[bugfix] Modified module implementations and documentation based off of identified issues (DCNE-160) #679
Conversation
…dency for aci_bulk_static_binding_to_epg
…ncy for aci_dhcp_relay_provider
…pendency for aci_vlan_pool
wiebecoding
requested review from
lhercot,
shrsr,
akinross,
anvitha-jain,
sajagana,
samiib and
gmicol
plugins/modules/aci_system_banner.py
Outdated
| @@ -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: | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Got it! I have added a test to ensure that the module failure does not occur. Thanks!
There was a problem hiding this comment.
Tests have failed with the current code changes.
There was a problem hiding this comment.
Whoops! Looks like I forgot to push the change to the text. Thanks for catching that! I will add it right now.
plugins/modules/aci_system_banner.py
Outdated
| if gui_banner is not None: | ||
| if re.fullmatch(regex_url, gui_banner): | ||
| is_gui_message_text = "no" | ||
| else: | ||
| is_gui_message_text = "yes" | ||
| else: | ||
| is_gui_message_text = "yes" | ||
|
|
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
Yes it can! Thank you!
There was a problem hiding this comment.
Whoops! Looks like I forgot to push the change to the text. Thanks for catching that! I will add it right now.
plugins/modules/aci_system_banner.py
Outdated
| @@ -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: | |||
There was a problem hiding this comment.
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.
plugins/modules/aci_system_banner.py
Outdated
| if gui_banner is not None: | ||
| if re.fullmatch(regex_url, gui_banner): | ||
| is_gui_message_text = "no" | ||
| else: | ||
| is_gui_message_text = "yes" | ||
| else: | ||
| is_gui_message_text = "yes" | ||
|
|
There was a problem hiding this comment.
Yes it can! Thank you!
|
All changed modules passed integration tests. |
github-actions
bot
changed the title
…dule does not fail for aci_system_banner
No description provided.