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
Fix nxos_vrf issues #37092
Fix nxos_vrf issues #37092
Conversation
@@ -266,7 +266,7 @@ def map_obj_to_commands(updates, module): | |||
commands.append('shutdown') | |||
commands.append('exit') | |||
|
|||
if interfaces: | |||
if interfaces and interfaces[0] != 'default': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why check only the first item of the list not to be default
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interfaces is a list parameter. It allows either an array of interfaces or 'default' keyword (to remove all the existing interfaces on the device). Since it is list, I am checking the first element to see if it is 'default' or not.
I followed this example:
if peer_list[0] == 'default': |
If there is some other better way, let me know.
@@ -293,7 +297,7 @@ def map_obj_to_commands(updates, module): | |||
commands.insert(0, 'vrf context {0}'.format(name)) | |||
commands.append('exit') | |||
|
|||
if interfaces: | |||
if interfaces and interfaces[0] != 'default': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question for this like the above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my reply above
@@ -318,6 +322,14 @@ def map_obj_to_commands(updates, module): | |||
commands.append('interface {0}'.format(i)) | |||
commands.append('no switchport') | |||
commands.append('no vrf member {0}'.format(name)) | |||
elif interfaces and interfaces[0] == 'default': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why check only the first item of the list to be default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my reply above
@@ -318,6 +322,14 @@ def map_obj_to_commands(updates, module): | |||
commands.append('interface {0}'.format(i)) | |||
commands.append('no switchport') | |||
commands.append('no vrf member {0}'.format(name)) | |||
elif interfaces and interfaces[0] == 'default': | |||
if obj_in_have['interfaces']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if there is no obj_in_have['interfaces']
? there should be an else
part in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, if interfaces is 'default' and the current object has some interfaces, this code removes them by doing 'no' cmd on each of them. There is no need for 'else' here because if the current object has no interfaces and user wants 'default', we are already in that state so no new commands need to be generated and impotence is maintained.
@trishnaguha Thanks for review comments, I put explanations for your remarks. Let me know if that is ok. Also please check my Note in the "Additional Information" section about the nxos_vrf_interface and decide whether we need to deprecate it. |
@saichint Thanks for the explanation. The PR looks good to me. |
cherry-picked to 2.5 |
* fix nxos_vrf issues * fix doc (cherry picked from commit dc61f4c)
* Fix nxos_switchport (#37328) (cherry picked from commit ff57fd0) * Fix nxos_l2_interface and test typo (#37336) Signed-off-by: Trishna Guha <trishnaguha17@gmail.com> (cherry picked from commit ba5e562) * fix ios_l2_interface (#37389) Signed-off-by: Trishna Guha <trishnaguha17@gmail.com> (cherry picked from commit dd37857) * fix required args for nxos_snapshot and docs improvement (#37232) Signed-off-by: Trishna Guha <trishnaguha17@gmail.com> (cherry picked from commit a10df8b) * add nxos_snapshot test for missing required param (#37248) Signed-off-by: Trishna Guha <trishnaguha17@gmail.com> (cherry picked from commit 2501834) * Ensure network_cli nxos test is run only once - remove unnecessary files (#37462) Signed-off-by: Trishna Guha <trishnaguha17@gmail.com> (cherry picked from commit 957ab02) * Integration Tests only: nxos_user (#37852) * add new integration file to nxos_user * fix shippable error * change nxapi to connection * review comments (cherry picked from commit 63da50e) * fix UnboundLocalError nxos_bgp_af module (#37610) Signed-off-by: Trishna Guha <trishnaguha17@gmail.com> (cherry picked from commit 00abe6d) * Fix nxos_vrf issues (#37092) * fix nxos_vrf issues * fix doc (cherry picked from commit dc61f4c) * fix nxos_vrf_af issues (#37211) (cherry picked from commit 74e79d9) * fix nxos_udld issues (#37418) (cherry picked from commit 05b266c) * fix nxos_vlan issues (#38008) (cherry picked from commit 6f2cb28) * add changelog Signed-off-by: Trishna Guha <trishnaguha17@gmail.com>
SUMMARY
Fixes #37091
ISSUE TYPE
COMPONENT NAME
nxos_vrf
ANSIBLE VERSION
ADDITIONAL INFORMATION
* NOTE: 'interfaces' parameter is added in 2.5 to nxos_vrf module. This does the same functionality as nxos_vrf_interface module. Actually nxos_vrf module is accepting 'list' for 'interfaces' which is more accurate than nxos_vrf_interface which takes only interface per task. Given these restrictions and redundancy, it is RECOMMENDED to deprecate nxos_vrf_interface module. If not, at least properly document this behavior in nxos_vrf_interface module.