public
public copied to clipboard
openconfig
Adjust enum variant values for consistency
- (M) release/models/mpls/openconfig-mpls-types.yang
- (M) release/models/qos/openconfig-qos.yang
- (M) release/models/qos/openconfig-qos-elements.yang
- (M) release/models/qos/openconfig-qos-mem-mgmt.yang
- (M) release/models/qos/openconfig-qos-interfaces.yang
- Remove enum variant values where there is only partial definition within the enumeration and there is no need for underlying mapping for functionality
- Add any missing variant values where applicable. If it falls outside of a protocol defined range, use negative integers to reflect (e.g. -1)
Most enum variant value declarations carry some meaning to an underlying specification and do not need to be defined in many cases. Per RFC6020, Section 9.6.4.2, if values are not specified, they will be automatically assigned. This PR removes any partial declarations that do not need to be there while adding any partial declarations that should. In the event that a variant value is defined that falls outside of a specification (e.g. for the ANY/ALL/NONE, catchall case), then a negative value is assigned as the values are within the range -2147483648 - 2147483647.
Sorry - meant to add a top-level comment.
Are these being removed because they are harmful, or because they aren't consistently used through every enumeration? If it's the latter, if it results in backwards incompatible changes, I'd like us to think through whether we have to change this, there seem like a number of places where /maybe/ we could have been more consistent, which we have not changed, and actually impact users. This case doesn't seem to.
Sorry - meant to add a top-level comment.
Are these being removed because they are harmful, or because they aren't consistently used through every enumeration? If it's the latter, if it results in backwards incompatible changes, I'd like us to think through whether we have to change this, there seem like a number of places where /maybe/ we could have been more consistent, which we have not changed, and actually impact users. This case doesn't seem to.
From my point of view - this is a low hanging fruit clean-up/consistency/protocol exercise only and should be non-affecting to most cases of backwards compatibility (esp. wire protocol) and purely an implementation detail issue when using assigned values (how this came about to be discovered).
I'd be curious how many folks are using the values and of what significance this provides especially in these partially defined scenarios otherwise I'd be for dropping assigned values altogether if we want to take that consistent approach.
Backwards incompatible per YANG definition but I'd be curious what if any backwards compat issues this produces in reality (reason why I did not increment the major version in this patch set initially) - are these of any significance in any codegen and how are undefined values accounted for in reality? Are they following YANG spec w/ auto-assignment?
I am not clear I agree that there are changes that are backwards incompatible that didn't have a major revision bump for models that had a major revision version greater than 0. If there are cases that we're not accounting for, please do highlight them so we can make sure to avoid them in the future.
w.r.t how the generated code that I am familiar with is handling this today, we would use the value that is found in the module to generate the AST that is provided by goyang, and then subsequently use the same value in our generated code. It's theoretically possible that a user can be calling the underlying integer value of the enum value - but I don't see users doing this today, since the names are the primary input.
@robshakir - I believe you are referencing my statement
RE: Backwards compatibility and the major version bump. If we look back on commit history, OpenConfig models to date have not followed any consistent rules of these sorts unfortunately - in fact there are various breaking changes that did not have a major version bump when they clearly should have - in other cases we have a major version bump when it may not have impacted clients.
There have definitely been cases but can take this as a side topic (IS-IS was one such domain I recall off the top of my head but would have to dig through) - some historical (as in 6 versions of a module ago still iterating on a 0.x.y w/ no major version bump but true breaks in compat) so if backwards incompatibilities are ok if the major version is on 0, I'm not sure I've seen this called out and it should deter folks from implementing anything before a 1.x.y version in reality (and signal to consumers to proceed w/ extreme caution)
Per IETF YANG 1.0/1.1 backwards compatibility rules, there are many changes that would constitute a major version bump (such as reordering an enum or altering variant values) but I think for OC models, we are currently determining possible impacts which can be subjective and CI is merely just checking if a version has been incremented but not if it was infact incremented correctly according to a determined pre-defined ruleset.
Thanks for the contribution. I support the idea that we should not introduce breaking changes for a consistency / cleanup use case. If there is a clear blocking issue, we could make breaking changes, but we haven't identified one that needs this change yet. So I recommend closing this PR.
