Update ocpaths.proto with releases and yang version #3545
Conversation
Add definition of Release as a timestamp plus two optionals Add to ocpath: origination, deprecation, removal yangModelVersion
Update ocpaths.proto with releases and yang version
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Pull Request Test Coverage Report for Build 11583791684Details
💛 - Coveralls |
There was a problem hiding this comment.
In general, it would be good to hear more on the motivation for these changes.
A concern I have is that the concept of nosimage.proto moves from a snapshot of supported features in a given release/platform combo to an abstract, database-like mechanism that keeps track of history of changes, some of which are not even related to vendor changes.
proto/ocpaths.proto
Outdated
| // not mutually exclusive with deprecation. | ||
| optional Release removal = 8; | ||
|
|
||
| // the YANG model version that first defined this path. this is typically |
There was a problem hiding this comment.
is there any value in tracking this for each vendor/sw/hw release? looks like vendors will need to maintain a database with such information in order export it..?
or is the expectation that the data will be provided by OC community and not used by vendors?
There was a problem hiding this comment.
Not the OC community - this is vendor-specific. I'll update the comments to make this clear.
That database would be the NOS images from previous releases. External logic would walk those
NOS images and calculate any origination/deprecation/removal dates to populate the current NOS
image. As a vendor, it is fairly common for our customers to ask for this information. But it is marked
as optional (I saw your later comment on this - I'll comment there) as a reminder that a vendor can
release a NOS image without this information.
There was a problem hiding this comment.
Not the OC community - this is vendor-specific.
// the YANG model version that first defined this path.
In that case perhaps I don't understand how the "yang model version" is defined. We, as a vendor, don't have any additional versioning of OC models. Or perhaps you're talking about your PR in general but not about the specific field?
That question is related to the field only (unfortunately I quoted the first line of the comment which didn't produce a nice preview). As for the overall PR reasoning, I left a comment above.
There was a problem hiding this comment.
that yangModelVersion typically identifies the OpenConfig consortium model that defines this leaf.
There was a problem hiding this comment.
that yangModelVersion typically identifies the OpenConfig consortium model that defines this leaf.
That brings me back to the original question: what is the value of tracking it per vendor/platform? :-)
There was a problem hiding this comment.
tracking this was a specific requirement handed to us by G
There was a problem hiding this comment.
@dplore would you be able to provide context?
| // the optional version of the release, typically numbers and dots. | ||
| optional string version = 2; | ||
|
|
||
| // the optional moniker (nickname) for the release | ||
| optional string name = 3; | ||
| } |
There was a problem hiding this comment.
featureprofiles/proto/nosimage.proto
Lines 39 to 43 in 13c8225
there are no nicknames for releases in nosimage.proto; also slightly different definition of the version field.
There was a problem hiding this comment.
Good point. The concept of the release triplet was not spread around to the rest of the
schema. You could rightly make the point that it should be. I have some reviewers coming
from G that I'll ask about that. Thanks for raising.
There was a problem hiding this comment.
per comments, this does have a constraint on the value to match /system/state/software-version
| google.protobuf.Timestamp date = 1; | ||
|
|
||
| // the optional version of the release, typically numbers and dots. | ||
| optional string version = 2; |
There was a problem hiding this comment.
in proto3 syntax there's no need to use optional syntax unless you need an explicit presence indicator for the field (any field can be omitted)
There was a problem hiding this comment.
TIL. Thanks for pointing that out. Boy do I have mixed feelings about that.
As a general critique, its fine if every field is optional but then IMO there needs to be some way to specify required (singly or for multiple attributes) and mutual exclusion. Oh, well. I wasn't on the proto committee when it was designed. :-)
I wanted to show that a timestamp was required and that the other two might be there or not. Any way to do this?
There was a problem hiding this comment.
Usually, comments (or a separate README/API contract document) are used to indicate mandatory fields when there's a need. I think you already did that in the parent comment (above the message Release). I don't think there's a way to enforce it in the syntax.
Although I wouldn't necessarily agree that specifying a release date without a release version is a great idea :)
and mutual exclusion.
this can be done via oneof
Changes based on feedback
clarified comment on yangModelVersion
Add a richer definition of a release identifier via new message, which has the original timestamp,
but adds an optional version number, which is typically how they are formally identified, plus an
optional moniker, which is how they are sometimes informally identified.
Add to ocpath three optional release identifiers: one for origination (the release when a path became
available from a vendor), one for deprecation (the release when a path is deprecated by a vendor),
and one for removal (when a path is no longer supported by a vendor).
Add to ocpath an optional yangModelVersion, which is typically the public YANG model version when
a path appears in a model, typically an OpenConfig consortium model - this is vendor independent.