Support leaf-list defaults using latest version of goyang. #618
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Note: I haven't released goyang yet since I want to release both repos at the same time. This is to avoid a downstream issue: openconfig/models-ci#59 - Update ygot to use the updated `Entry.Default []string` and its corresponding helpers for storing defaults for leaf-lists. - Change some helper function such as `yangDefaultValueToGo` to return `string` instead of `*string`. There is no good reason for returning a pointer and makes the code less convenient. - Factor out `generateGoDefaultValue` from `writeGoStruct`, and have it handle multiple default values. - In protogen's logic to generate default enums, check whether an enum is not a leaf-list before setting a particular one as the default enum value. Note: I'm not sure whether this is the correct handling -- though it appears to me that having a default value doesn't make sense for leaf-lists as repeated fields. Testing: - Added integration tests for Go and proto generation.
robshakir
approved these changes
Feb 17, 2022
|
@dplore I added you as another pair of eyes. Feel free to leave some high-level comments/questions. The context here is that goyang had a backwards-incompatible change (allowing defaults for leaf-lists): openconfig/goyang#139 This PR adapts ygot to be able to generate defaults for leaf-lists, see this integration test: |
|
I'm going to merge this with Rob's approval. |
Note: I haven't released the goyang commit this is dependent on yet (hence the go.mod replace statement) since I want to release both repos at the same time. This is to avoid a downstream issue: openconfig/models-ci#59
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note: I haven't released the goyang commit this is dependent on yet
(hence the go.mod replace statement) since I want to release both repos
at the same time. This is to avoid a downstream issue:
openconfig/models-ci#59
Entry.Default []stringand itscorresponding helpers for storing defaults for leaf-lists.
yangDefaultValueToGo()to returnstringinstead of*string. There is no good reason for returning apointer and makes the code less convenient.
generateGoDefaultValue()fromwriteGoStruct(), and have ithandle multiple default values.
is not a leaf-list before setting a particular one as the default enum
value.
Note: I'm not 100% sure whether this is the correct handling -- it
appears to me that having a default enum value doesn't make sense for
leaf-lists as repeated fields.
Testing: