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

path format and root format #13

Closed
slieberth opened this issue Apr 26, 2021 · 5 comments
Closed

path format and root format #13

slieberth opened this issue Apr 26, 2021 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@slieberth
Copy link
Contributor

Hi Anton,

I verified the pygnmi path syntax and I am not sure, whether the pygnmi path generator implementation matches the gnmi spec ...

in client.py docstring the path is specified_
path = ['yang-module:container/container[key=value]', 'yang-module:container/container[key=value]', ..]

in https://github.com/openconfig/reference/blob/master/rpc/gnmi/gnmi-path-conventions.md I see following spec: ... the root path / is encoded as a zero length array (slice) of PathElem messages. Example declarations in several languages: Python: path = [] Note this is not the same as a path consisting of a single empty string element. A human-readable path can be formed by concatenating elements of the prefix and path using a / separator, and preceded by a leading / character. ....

in this respect I think we should verify the pygmni implementation:

  • a single root path / should be allowed. The current pygnmi implementation incorrectly converts "/" to "path { elem {} elem {} }", should be changed to "path []"
  • leading / character must be allowed in path. This is not the case with current implementation. E.g. the current pygnmi implementation incorrectly converts "/interfaces/interface" to "path { elem {} elem { name: "interfaces"} elem { name: "interface } }", should be changed to "path { elem { name: "interfaces"} elem { name: "interface } }""
  • both two options must allow origin specification: e.g. "openconfig:/" or "openconfig:interfaces:/interfaces/interface"

@anton, I am happy to change the path generator model accordingly, test against Juniper and Arista and send a pull request ... let me know ...

best regards Stefan

@akarneliuk
Copy link
Owner

Hello @slieberth ,

Actually, that was a thing I added to the new release published yesterday (modification of the path processing to allow '/' and possibility to omit or add origin). Could you please check that out?

Best,
Anton

@slieberth
Copy link
Contributor Author

Hi Anton,

thanks for the changes, looks almost perfect, if origin and leading slash is present (e.g. openconfig-interfaces:/interfaces/interface), then origin is not detected correctly, instead the leading origin gets a path element ...

/ --> path {} --> OK!

/interfaces/interface --> path { elem {name: "interfaces"} elem {name: "interface" } --> OK!

openconfig-interfaces:interfaces/interface --> path { origin: "openconfig-interfaces" elem { name: "interfaces" } elem { name: "interface" } } --> OK!

openconfig-interfaces:/interfaces/interface --> path { elem { name: "openconfig-interfaces:" } elem { name: "interfaces" } elem { name: "interface" } --> NOK!

regards Stefan

@akarneliuk akarneliuk self-assigned this Apr 27, 2021
@akarneliuk akarneliuk added the bug Something isn't working label Apr 27, 2021
@akarneliuk
Copy link
Owner

Hey @slieberth ,

I think it is fixed now. Could you please check?
Could you please also point to the part in documentation with that part : openconfig:interfaces:/interfaces/interface. I tried to search, but not found, so built on your advise.

Best,
Anton

@slieberth
Copy link
Contributor Author

Hi Anton,
thanks for the change, looks good, can confirm the fix, thanks.

in respect to documentation: the doc-string in client.py:148 needs minor change:
path = ['yang-module:container/container[key=value]', 'yang-module:container/container[key=value]', ..]

maybe you should specify also a path without origin prefix. In my tests against arista, origin is completly ignored, so also xxx:/interfaces/interface does work. When testing against Cisco and Juniper I use only the path, without origin.

Greetings from Berlin
Stefan

@akarneliuk
Copy link
Owner

Hello @slieberth ,

Thanks a lot. We'll add the mentioned enhancement in the next release in the documentation. With this, we will close the issue.

Best,
Anton

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants