-
Notifications
You must be signed in to change notification settings - Fork 50
Fix issue 538 create vrf wo vni #573
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
base: develop
Are you sure you want to change the base?
Fix issue 538 create vrf wo vni #573
Conversation
Merge branch 'develop' with 'fix_issue_538_create_vrf_wo_vni'
|
@allenrobel / @mikewiebe: This PR is ready for review. |
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.
Pull request overview
This PR fixes an issue where creating a new VRF without specifying a L3VNI (vlan_id) fails on ND version 4.1, while working on versions 3.1 and 3.2. The fix ensures compatibility across all three ND versions by transforming the default vrfVlanId value from 0 to an empty string ("") for DCNM version 12+.
Key Changes
- Fixed a bug in
get_next_vrf_id()where the retry loop would continue even after successfully retrieving a vrf_id, potentially overwriting valid values - Modified
diff_merge_create()to transformvrfVlanIdfrom0to""for DCNM version 12+, ensuring compatibility with ND 4.1
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
plugins/modules/dcnm_vrf.py
Outdated
| template_conf.update( | ||
| vrfVlanId="" if template_conf["vrfVlanId"] == 0 else template_conf["vrfVlanId"] |
Copilot
AI
Dec 3, 2025
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.
[nitpick] The inline conditional in the update() call is difficult to read and maintain. Consider extracting this logic to a separate variable before the update:
vrf_vlan_id_value = "" if template_conf["vrfVlanId"] == 0 else template_conf["vrfVlanId"]
template_conf.update(vrfVlanId=vrf_vlan_id_value)This improves readability and makes the transformation logic more explicit.
| template_conf.update( | |
| vrfVlanId="" if template_conf["vrfVlanId"] == 0 else template_conf["vrfVlanId"] | |
| vrf_vlan_id_value = "" if template_conf["vrfVlanId"] == 0 else template_conf["vrfVlanId"] | |
| template_conf.update( | |
| vrfVlanId=vrf_vlan_id_value |
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.
Implemented suggestion in ^9608cc6
plugins/modules/dcnm_vrf.py
Outdated
| if self.dcnm_version > 11: | ||
| template_conf.update(isRPAbsent=json_to_dict.get("isRPAbsent")) | ||
| template_conf.update( | ||
| vrfVlanId="" if template_conf["vrfVlanId"] == 0 else template_conf["vrfVlanId"] |
Copilot
AI
Dec 3, 2025
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.
The conditional check template_conf["vrfVlanId"] == 0 doesn't handle the case where vrfVlanId might be None. When json_to_dict.get("vrfVlanId") is called at line 2246 without a default value, it returns None if the key is missing.
Consider using a more defensive check:
template_conf.update(
vrfVlanId="" if template_conf.get("vrfVlanId") in [0, None] else template_conf["vrfVlanId"]
)Or alternatively, ensure vrfVlanId has a default value at line 2246:
"vrfVlanId": json_to_dict.get("vrfVlanId", 0),| vrfVlanId="" if template_conf["vrfVlanId"] == 0 else template_conf["vrfVlanId"] | |
| vrfVlanId="" if template_conf.get("vrfVlanId") in [0, None] else template_conf["vrfVlanId"] |
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.
Implemented the defesive check in ^9608cc6
allenrobel
left a comment
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.
Hi Miguel, this looks fine to me. I've also enabled Copilot review which generated a couple comments that look worth considering.
Thanks!
| ) | ||
| template_conf.update( | ||
| isRPAbsent=json_to_dict.get("isRPAbsent") | ||
| ) |
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.
Is the update for isRPAbsent related to creating the VRF without VNI or is it a separate issue? It's fine that it's added here, but am rather just curious about it.
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.
It's not related. I just added the newline to standirize the elements inside the update function.
This PR fixes an issue when creating a new VRF without specifying a L3VNI.
The issue was with the
vrfVlanIdparameter. When the value wasn't provided in the playbook, a default value of0was ussed. Now, if thevrfVlanIdparameter is not provided, we transform it to an empty string (""). This default value works accross all 3 versions of ND.Fix #538