Skip to content
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

Support explicitly setting proto field tags. #549

Closed
wants to merge 14 commits into from
Closed

Support explicitly setting proto field tags. #549

wants to merge 14 commits into from

Conversation

wenovus
Copy link
Collaborator

@wenovus wenovus commented Jun 21, 2021

Credits to @jasonewang for posting the first implementation at #339. I've modified this a bit to read the extension values from the new openconfig-codegen-extensions.yang model that's now in openconfig/public, and also to clarify the corner cases that were discussed in the PR.

Spec:
https://github.com/openconfig/ygot/blob/master/docs/yang-to-protobuf-transformations-spec.md#field-numbering

Corner cases not mentioned in the spec:

  1. multiple field-number extensions.
    • Error out.
  2. multiple field-number-offset extensions.
    • Add them up.
  3. field-number or field-number-offset extensions on non-leaves.
    • Silently ignore.
  4. field-number-offset on non-uses statements, including leafs.
    • Allow.
  5. Conflicts.
    • Allow. The proto compilation will of course fail. Automated checking can be added here if we'd prefer.

I made some simple choices for the above items. Erroring out for 3 and 4 would likely require changing goyang to specifically support checking these cases, which I'm not sure is worth it here.

jasonewang and others added 13 commits December 20, 2019 11:38
The YANG to Protobuf specification mentions using extensions to explicitly set
the field tag of generated Protobuf fields. Add support for annotating field
tags.

* Support setting the field number with occodegenext:field-number.

* Support offsetting the field number with occodegenext:field-number-offset.

* Reject field number annotations that result in a value in the Protobuf
reserved range.
Credits to @jasonewang for posting the first implementation at #339. I've
modified this a bit to read the extension values from the new
openconfig-codegen-extensions.yang model that's now in
openconfig/public, and also to address some of the corner cases that
were discussed in the PR.

Spec:
https://github.com/openconfig/ygot/blob/master/docs/yang-to-protobuf-transformations-spec.md#field-numbering

Corner cases not mentioned in the spec:
1. multiple `field-number` extensions.
  - Error out.
1. multiple `field-number-offset` extensions.
  - Add them up.
1. `field-number` or `field-number-offset` extensions on non-leaves.
  - Silently ignore.
1. `field-number-offset` on non-uses statements, including leafs.
  - Allow.
1. Conflicts.
  - Allow. The proto compilation will of course fail. Automated checking can be added here if we'd prefer.

I made some simple choices for the above items. Erroring out for 3 and 4 would
likely require changing `goyang` to specifically support checking these cases,
which I'm not sure is worth it here.
@wenovus wenovus requested a review from robshakir June 21, 2021 22:55
@google-cla
Copy link

google-cla bot commented Jun 21, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no This PR author has not signed the CLA label Jun 21, 2021
@coveralls
Copy link

coveralls commented Jun 21, 2021

Coverage Status

Coverage decreased (-0.03%) to 90.577% when pulling ac8c688 on pr/339 into 926dd30 on master.

@wenovus
Copy link
Collaborator Author

wenovus commented Jun 21, 2021

@jasonewang would you be able to consent to this PR being submitted based on your prior commits? (please see googlebot's post)

@wenovus
Copy link
Collaborator Author

wenovus commented Jun 24, 2021

@googlebot I consent

@wenovus
Copy link
Collaborator Author

wenovus commented Jun 25, 2021

@jasonewang Do you think you can consent to your commits being contributed to this project? If so could you consent like I did above? Thanks a lot.

@wenovus
Copy link
Collaborator Author

wenovus commented Jun 25, 2021

Ok, I figured out you can push to the fork repository. So I just did that instead.

@wenovus wenovus closed this Jun 25, 2021
@wenovus wenovus deleted the pr/339 branch June 25, 2021 22:04
@wenovus wenovus restored the pr/339 branch June 25, 2021 22:04
@wenovus wenovus deleted the pr/339 branch March 24, 2022 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: no This PR author has not signed the CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants