openconfig / reference

This repository contains reference implementations, specifications and tooling related to OpenConfig-based network management.
Apache License 2.0
155 stars 88 forks source link

Clarify gNMI SetRequest `update` when updating a keyed list. #175

Closed wenovus closed 1 year ago

wenovus commented 1 year ago

Added example, and moved around a paragraph to a more related/discoverable place.

earies commented 1 year ago

@wenovus @robshakir - can we at least devise a protocol around specification updates that they are tracked and recorded in a changelog, version numbers, dates, etc... are updated in a meaningful fashion (and tied to the proto IDL versioning which differs from the spec)?

otherwise you have a specification that is constantly being changed out from underneath having to rely off SCM tools only to determine what and why something was altered

dplore commented 1 year ago

Perhaps we should we ensure we sync the semantic version of the proto file and and specification? Would this address the concern? (I realize this PR did not increment the version of the spec, much less the proto file, even though the proto file had no change)

wenovus commented 1 year ago

Apologies for forgetting to update the version number. One simple thing to do is to add a GitHub Action to remind the PR author/reviewer to update the version in the markdown file. We can also write a Markdown parser, or just a simple regex, to check that the version number updated correctly.

I don't think the version number can exactly match that of the proto file (I presume you mean this one), since spec updates are sometimes just clarifications (e.g. this PR) which did not necessitate any changes in gnmi.proto. However, we could keep the minor version the same and make a comment in the spec describing this association.

earies commented 1 year ago

Perhaps we should we ensure we sync the semantic version of the proto file and and specification? Would this address the concern? (I realize this PR did not increment the version of the spec, much less the proto file, even though the proto file had no change)

We can't necessarily do that - the proto IDL is more static and iterates less while the specification is constantly evolving as implementation and/or adoption increases as we are finding more and more "undefined or ambiguous" behavior.

I do believe there should be a relationship between the IDL and the specification however - much like YANG related RFCs tie in the model to the specification. In this case, they are detached and a specification version may need to be tied to an IDL version in lock-step

The versioning issue currently exists across various projects especially when implementors are tasked with "must support gNMI" - there needs to be a concrete relationship as to what version of APIs, models and specifications otherwise its an ever moving goal post

dplore commented 1 year ago

Perhaps to keep the proto and spec in sync, we could perform versioning this way:

Clarifications to the spec could be treated semantically as a patch revision and incremented on both spec and proto.

Changes in behavior or intent in the spec which are backwards compatible in the proto (including no change to the proto content) could likewise be semantically incremented as a minor change.

Changes in behavior or intent in the spec which are not backwards compatible in the proto would increment the semantic major version.

wenovus commented 1 year ago

This would require us to bump the patch version of gnmi.proto whenever the spec changes in any way, including a minor clarification. I'm currently leaning towards having a sentence that says "spec compatible with gnmi.proto version 0.8.x".

This is clearly fine when we're just doing a spec clarification update, and would require us to be careful with gnmi.proto version updates such that new features always increment the minor version as well even though we're pre-v1.0.0, but this avoids us having to maintain lockstep for minor changes in either repository.

I'm curious whether gnmi.proto should be at v1.0.0 since it looks like users are depending more and more on the API and we're starting to worry about backwards compatibility (e.g. the recent double change in gnmi.proto)?

earies commented 1 year ago

It's good that this sparked off great conversation but we may want to move this thread (or at least back ref it) somewhere else otherwise we are going to have a very hard time finding this history at a later date.

+1 for @wenovus comment above as I think we just need to draw a relationship between the specification and IDL. The specification is filling in behavioral gaps over time that don't necessarily change message structs or service RPCs

But I'd also like to call this out for all projects that fall under the OpenConfig umbrella and we should drive this consistent approach across all. To date, this should apply to:

Across the various repos (gribi, gnoi, grpctunnel, public, gnmi, etc..) - we have various inconsistencies of above

I'm curious whether gnmi.proto should be at v1.0.0 since it looks like users are depending more and more on the API and we're starting to worry about backwards compatibility (e.g. the recent double change in gnmi.proto)?

The blunt reality is version scheme aside, dependencies have been created since ~0.4.0 of the IDL due to this shipping in mainline code in various implementations so the worry has always been there

As far as bumping to 1.0.0, I know at least there are a handful of considerations (and some deprecation of fields that were never implemented/used) that should probably get addressed before we bump to a major version that indicates greater stability. Some of these currently sit as idle or unaddressed PRs/issues in the gNMI repo.