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

gNMI Depth Extension documentation #205

Merged
merged 5 commits into from
Mar 27, 2024
Merged

Conversation

hellt
Copy link
Contributor

@hellt hellt commented Mar 9, 2024

Documentation entry to compliment openconfig/gnmi#166

Rendered doc can be seen here.

@hellt
Copy link
Contributor Author

hellt commented Mar 9, 2024

@robshakir @dplore here is a documentation entry for gnmi depth

rpc/gnmi/gnmi-depth.md Outdated Show resolved Hide resolved
rpc/gnmi/gnmi-depth.md Outdated Show resolved Hide resolved
@hellt
Copy link
Contributor Author

hellt commented Mar 15, 2024

Hi @dplore @robshakir
Maybe someone can give it a quick readout? Nothing changed from the original PR description, so should look and feel familiar.

@hellt
Copy link
Contributor Author

hellt commented Mar 25, 2024

Hi @dplore @robshakir
Just wanted to make sure this has not slipped your radars. Would be nice to close on that in the near term if possible.

@@ -0,0 +1,271 @@
# gNMI Depth Extension
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you wrap the doc to 80 chars please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @robshakir

I also addressed other review comments in f28e8fa

rpc/gnmi/gnmi-depth.md Outdated Show resolved Hide resolved

Value of 1 means only the specified path and its direct children will be returned. See Children section for more info.

#### 3.1.2 Value of N+
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe word this just as Value of N -- I'm not quite sure I understand the + here, and since its a uint32, it's not like we can include the + in the value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call, will address this


### 3.2 Children nodes

The Depth extension operates the value of "direct children of a schema node". What we understand by direct children:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since gNMI covers more than YANG-modelled data, then I think it is desirable to have the specification apply to data that is not YANG modelled. Particularly, we could do this by specifying scalar values, and arrays of scalar values. You can tie it to the examples in YANG if desired.

It's probably equally valid to say that any node in the tree that is a leaf node is returned, and explicitly nodes that are the root of a subtree are not returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, will add an interpretation of this


## 4 Examples

Using the data model from Section 2 we will run through a set of examples using the patched version of [openconfig/gnmic](https://gnmic.openconfig.net/) client with the added Depth extension support. We can provide the patched gnmic binary for Linux x86_84 if you want to try it out.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest removing the "we can provide..." sentence for the specification here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apoligies, this was a leftover from the original issue description

hellt and others added 2 commits March 26, 2024 20:13
Co-authored-by: Rob Shakir <robjs@google.com>
Copy link
Contributor

@robshakir robshakir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes appreciated -- LGTM.

@robshakir robshakir merged commit 12ded5a into openconfig:master Mar 27, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants