-
Notifications
You must be signed in to change notification settings - Fork 47
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
feat: Add BGP Metrics fields for fabric cloud router routing protocols #794
Conversation
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.
LGTM!
Need to fix Lint Validation and Go build
@@ -712,6 +748,16 @@ func routingProtocolBgpIpv4TerraformToGo(routingProtocolBgpIpv4Request []interfa | |||
enabled := bgpIpv4Map["enabled"].(bool) | |||
rpBgpIpv4.SetEnabled(enabled) | |||
|
|||
if outboundAsPrependCount := bgpIpv4Map["outbound_as_prepend_count"].(int); outboundAsPrependCount > 0 { |
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.
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.
I realized that 0 won't be included then. Is there a different reason for outboundAsPrependCount > 0
?
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.
Ah, good find on the wiki. This creates a problem though. SDKv2 always assigns the zero value of the type and doesn't have the concept of null
for an unassigned value. Zero value for int is always 0, and I wasn't trying to avoid sending the value in the API request if the user didn't explicitly include it in the config.
Might have to rework how we can handle this case in SDKv2; or we don't support this feature until we can move to terraform-plugin-framework.
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.
I have a plan that can work though. We can use strings for these values and convert internally during mapping.
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.
That could work.
In that case, if it's a string "0" (in SDKv2) it would mean an actual value of outboundAsPrependCount
and if it is int 0, then it would mean 0 (for unassigned value/null).
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.
Let me rethink this.
You said we can use strings and convert during mapping, but then what about int 0 values (unassigned values)
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.
Never mind, I understood how you can implement this in the code.
@d-bhola , updated it to string type to account for user possibly not adding the parameter and to account for allowing the user to choose zero explicitly. |
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.
LGTM
This PR is included in version 2.7.0 🎉 |
#794) * Added additional optional fields to routing protocols bgp_ipv4 and bgp_ipv6 objects * Included acceptance testing Acceptance Tests for Fabric will be failing intermittently because in UAT the Vlan Tags are not being released quickly (or possibly at all). I've included evidence of local passing tests in a screenshot below: <img width="1227" alt="image" src="https://github.com/user-attachments/assets/74c340b8-7957-4dcf-88d2-ba2cdd8147b2">
Acceptance Tests for Fabric will be failing intermittently because in UAT the Vlan Tags are not being released quickly (or possibly at all). I've included evidence of local passing tests in a screenshot below: