feat: support for diffing presence containers #990
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
JonasKs
force-pushed
the
support-presence-containers
branch
from
4970f67 to
519ff68
Compare
|
Hello @wenovus and @robshakir - it took some time, but we finally managed to crack the code. Please review. 😊 |
JonasKs
force-pushed
the
support-presence-containers
branch
from
519ff68 to
58943fa
Compare
Co-authored-by: Terje Lafton <[email protected]>
JonasKs
force-pushed
the
support-presence-containers
branch
from
58943fa to
8c252eb
Compare
Co-authored-by: Terje Lafton <[email protected]>
|
Also tagging #645 author, @hansthienpondt. |
Co-authored-by: Terje Lafton <[email protected]>
|
Hi @robshakir , friendly bump in case you missed this 😊 |
|
I did miss it -- apologies, let me review ASAP. |
No worries, @robshakir! We’ve been using the fork in production and it’s worked like a charm. |
|
@robshakir, would love to get this looked at. 😊 |
|
@JonasKs holy-moly! apologies! this one passed me by. reviewing now. 🔎 |
| @@ -193,6 +193,16 @@ func getOrderedDirDetails(langMapper LangMapper, directory map[string]*Directory | |||
| } | |||
| default: | |||
| pd.Type = Container | |||
| if len(dir.Entry.Extra["presence"]) > 0 { | |||
There was a problem hiding this comment.
filed openconfig/goyang#286 since this is a little ugly. Can you add a TODO -- i.e.,:
TODO(https://github.com/openconfig/goyang/issues/286): update to use direct yang.Entry field once presence is natively supported in goyang.
| if v := dir.Entry.Extra["presence"][0].(*yang.Value); v != nil { | ||
| pd.PresenceContainer = true | ||
| } else { | ||
| return nil, fmt.Errorf( | ||
| "unable to retrieve presence statement, expected non-nil *yang.Value, got %v", | ||
| dir.Entry.Extra["presence"][0], | ||
| ) | ||
| } |
There was a problem hiding this comment.
| if v := dir.Entry.Extra["presence"][0].(*yang.Value); v != nil { | |
| pd.PresenceContainer = true | |
| } else { | |
| return nil, fmt.Errorf( | |
| "unable to retrieve presence statement, expected non-nil *yang.Value, got %v", | |
| dir.Entry.Extra["presence"][0], | |
| ) | |
| } | |
| if v := dir.Entry.Extra["presence"][0].(*yang.Value); v == nil { | |
| return nil, fmt.Errorf("unable to retrieve presence statement, expected non-nil *yang.Value, got %v", dir.Entry.Extra["presence"][0]) | |
| } | |
| pd.PresenceContainer = true |
since this uses the go-style approach of exiting early for errors and avoiding indentation. it also fixes the go-style point of not having linebreaks @80ch.
| // we set it's value to `nil` instead of returning earlier. | ||
| // This is because empty presence containers has a meaning, | ||
| // unlike a normal container. | ||
| outs[vp] = nil |
There was a problem hiding this comment.
In an empty container, isn't ival already set to nil? it's specified as ni.FieldValue.Interface() above.
| @@ -426,6 +437,24 @@ func hasIgnoreAdditions(opts []DiffOpt) *IgnoreAdditions { | |||
| return nil | |||
| } | |||
|
|
|||
| // The RespectPresenceContainers DiffOpt indicates that presence containers | |||
| // should be respected in the diff output. | |||
| // This option was added to ensure we do not break backward compatibility. | |||
There was a problem hiding this comment.
And it's appreciated! :-)
|
Thanks so much, @robshakir! I’m out of office for ~2 weeks, but I’ll address all comments when I’m back! |
Closes #936.
The implementation ended up being somewhat as described in #936, where we indicate that presence container has a
nilvalue in thefindSetLeavesfunction.In addition to this, we added a presence container interface, which is used in the
marshalStructOrOrderedListto isolate the presence container which might have values, to act like an empty struct.The result is that we do not break any current tests (even without the diff option), but we decided to add a diff option, with the reasoning described in #936:
We've confirmed this works in our setup. We're able to successfully configure all presence containers, even nested ones.
Co-authored-by: Terje Lafton [email protected]