-
Notifications
You must be signed in to change notification settings - Fork 16
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
Model bgpsrte restruct #172
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.
In general , looks good to me. Only one attribute related clarification needed for local_preference i.e. whether we can have a default working mode for iBGP use-case which is the default and extra option to not include it optionally for eBGP. Rest are all related to descriptions.
What issue or enhancement is this PR addressing? Can we link the PR to it? And is this change backwards incompatible? |
https://github.com/open-traffic-generator/models/pull/154 |
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.
@SuryyaKrJana saw your comments. Answers my questions. I'm ok to take this in. You don't need to wait for my review. I would expect that BGP domain experts are reviewing in more detail.
Noted.
Hi
I think it might be better to start with known types.
If the concrete definition of the hex type is known , we can add that as needed later.
Is the hex value needed from RFC support point of view or is there some known use-case we can’t support without being able to specify hex values ?
Regards
Apratim
From: anirbanb07 ***@***.***>
Sent: 27 September 2021 08:04
To: open-traffic-generator/models ***@***.***>
Cc: Apratim Mukherjee ***@***.***>; Review requested ***@***.***>
Subject: Re: [open-traffic-generator/models] Model bgpsrte restruct (#172)
CAUTION: This message originates from an external sender.
@anirbanb07 commented on this pull request.
________________________________
In device/bgp/bgpexcommunity.yaml<https://urldefense.com/v3/__https:/github.com/open-traffic-generator/models/pull/172*discussion_r716320422__;Iw!!I5pVk4LIGAfnvw!zz6UcNsJkZAUMq6SfN6kdCTz2Zta4kPOMdEygq2k3asUP3jo4FFn8iAOzXHfAfo-oN1WC_yY$>:
+ - administrator_ipv4_address
+ - administrator_as_4octet
+ - opaque
+ - evpn
+ - administrator_as_2octet_link_bandwidth
+ subtype:
+ description: |-
+ Extended Community Sub Type field of 1 Byte.
+ - route_target: Route Target.
+ - origin: Origin.
+ - extended_bandwidth: Specifies the link bandwidth.
+ - color: Specifies the color value.
+ - encapsulation: Specifies the Encapsulation Extended Community.
+ - mac_address: Specifies the Extended community MAC address.
+ type: string
+ enum:
Can we have a provision here that user can enter any hex value here apart from these few types/subtypes?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https:/github.com/open-traffic-generator/models/pull/172*pullrequestreview-763791729__;Iw!!I5pVk4LIGAfnvw!zz6UcNsJkZAUMq6SfN6kdCTz2Zta4kPOMdEygq2k3asUP3jo4FFn8iAOzXHfAfo-oD4oW33U$>, or unsubscribe<https://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/ASZ6MO64PGXCH6PN4I5VRWTUD7J25ANCNFSM5EWSQPPA__;!!I5pVk4LIGAfnvw!zz6UcNsJkZAUMq6SfN6kdCTz2Zta4kPOMdEygq2k3asUP3jo4FFn8iAOzXHfAfo-oFxqlqZu$>.
Triage notifications on the go with GitHub Mobile for iOS<https://urldefense.com/v3/__https:/apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675__;!!I5pVk4LIGAfnvw!zz6UcNsJkZAUMq6SfN6kdCTz2Zta4kPOMdEygq2k3asUP3jo4FFn8iAOzXHfAfo-oKKD50U5$> or Android<https://urldefense.com/v3/__https:/play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!I5pVk4LIGAfnvw!zz6UcNsJkZAUMq6SfN6kdCTz2Zta4kPOMdEygq2k3asUP3jo4FFn8iAOzXHfAfo-oA-Up0JW$>.
|
…ement changed the input format to hex
…ement changed the input format to hex
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.
Most of it is a documentation change, and I believe can be addressed as a separate bugfix.
@ajbalogh do you have any ideas on how to fix multiline comments involving bullets ? In redocly they all seem to appear as a single line making it harder to read the text. Maybe we could work out some rule for writing descriptions (and enhance bundler accordingly) ? |
@ashutshkumr I agree we should come up with a standard for representing descriptions. |
https://redocly.github.io/redoc/?url=https://raw.githubusercontent.com/open-traffic-generator/models/model_bgpsrte_restruct/artifacts/openapi.yaml
While working PR#154 at the end , we have found SRTE needs a refactoring a bit. We discussed in the scrum and found no body right now using SRTE model. All agreed to disable in SRTE from top level (BGP Peer) and merged PR#154 to master to progress Controller and Rustic working together. That’s testing is going on. Btw, I have created another PR#172 for SRTE Model modification. It is backward compatible to PR#154.
Previous PR: https://github.com/open-traffic-generator/models/pull/154
#Snappi and Json code examples.