Add fields to IR and IROptions used by settting the path origin from the YANG module name or a specified name. #1030
Conversation
…mand option to set the path origin from the YANG module name or an arbitrarily specified name.
keogaki
changed the title
There was a problem hiding this comment.
Thanks for the contribution -- but this doesn't look like the right design to me. We're adding things to the ygot-generated IR that are not really in-line with what the IR should be specifying -- i.e., pre-parsed details of YANG structures that code-generation can use.
ygen/ir.go
Outdated
| @@ -312,6 +312,13 @@ type IR struct { | |||
| // fakeroot stores the fake root's AST node for creating a serialized | |||
| // version of the AST if needed. | |||
| fakeroot *yang.Entry | |||
|
|
|||
| // UseModuleNameAsPathOrigin specifies whether the YANG module name is | |||
There was a problem hiding this comment.
This doesn't seem to make sense in the IR -- rather than storing the argument of the flag, we should rather provide information in the IR for each leaf that specifies what the origin that should be used is.
There was a problem hiding this comment.
As you suggested, I modified this to parse the origin name information during IR generation, remove the flag field from IR struct and move the origin name field under NodeDetails struct.
ygen/ir.go
Outdated
| // set to the origin for generated gNMI paths. | ||
| UseModuleNameAsPathOrigin bool | ||
|
|
||
| // PathOriginName specifies the orign name for generated gNMI paths. |
There was a problem hiding this comment.
| // PathOriginName specifies the orign name for generated gNMI paths. | |
| // PathOriginName specifies the origin name for generated gNMI paths. |
There was a problem hiding this comment.
I made a mistaken Re-request review. We will fix later.
ygen/genir.go
Outdated
| opts: opts, | ||
| fakeroot: rootEntry, | ||
| parsedModules: mdef.modules, | ||
| UseModuleNameAsPathOrigin: opts.UseModuleNameAsPathOrigin, |
There was a problem hiding this comment.
This doesn't seem the right information to be including in the IR -- you're strictly handing flags that are provided as options back to the caller, rather than doing the parsing for them.
Per the design here: https://github.com/openconfig/ygot/blob/master/docs/code-generation-design.md -- the IR is designed to abstract the details of the YANG away such that multiple downstream consumers can use the same representation.
A better design here would be to have ygen parse the information and populate a field in the IR that is specified per-node (leaf/container/list etc.) that determines what origin should be used for that particular field.
There was a problem hiding this comment.
@robshakir
Thanks for your prompt review.
From my understanding of the design, it seems appropreate to add the OriginName field to NodeDetails struct defined in ygen/ir.go and put the desired origin name there depending on the modified IROptions struct value in getOrderedDirDetails() function in ygen/directory.go.
Then, we can retrieve the origin name in pathgen/pathgen.go and remove the both added fields from IR struct.
Is this right approach?
There was a problem hiding this comment.
Yes, you can add an Origin field to the YANGNodeDetails struct in ygen/ir.go as one option. Alternatively, you already have BelongingModule there, which seems like it should include the contents that you want already?
There was a problem hiding this comment.
I added the origin name filed, PathOriginName, to NodeDetails struct instead of YANGNodeDetails struct, since the origin itself doesn't intirincically related to and isn't always specified by YANG schema. This PR includes not only to set YANG module name to the origin, but also set an arbitrarily name to all the nodes from generator command option.
As you suggested, IR shouldn't convey any option flag from the design policy. If we use the BelongingModule depending on the flag during the code generation, not the IR generation, this is same as the initial commit.
See ygnmi PR154.