Allow more control over gnmi set request marshalling#157
Allow more control over gnmi set request marshalling#157dplore merged 3 commits intoopenconfig:mainfrom
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. |
This patch extends the current behavior to populate the gnmi set request in order to allow more control over the request payload. 1. A new `WithAppendModuleName` option is added that allows to toggle the procedure of adding the YANG module name to the respective fields, which is redundant for systems that will automatically infer the module name via other means. 2. The `WithEncoding` option is respected when set to json encoding. Previously the typed value was marshalled by default as json_ietf, while certain platform such as Cisco NX-OS only support json enconding and therefore expect all request to be encoded as such. With this option, it's now possible to encode request to json if desired. All changes mentioned above do not alter the current behavior. They simply allow for more flexibility when used explicitly by providing the respective options.
Pull Request Test Coverage Report for Build 17956202556Details
💛 - Coveralls |
robshakir
left a comment
robshakir
left a comment
There was a problem hiding this comment.
Thanks for the change. Generally, LGTM -- added a couple of comments.
Can we add a test to validate these two new options in this package please?
Added some in 68dcb70. :) |
|
@robshakir I addressed your feedback, would you be able to have another look at this? |
robshakir
left a comment
robshakir
left a comment
There was a problem hiding this comment.
thanks for the changes, apologies for the delay re-reviewing.
|
/gcbrun |
|
Hey @felix-kaestner , thanks for doing this! My only nit is |
@eoswald Valid point. @robshakir what are your feelings? |
|
@robshakir Can we get another /gcbrun on this? I think it's stuck. Also any strong feelings on the discussion? |
|
/gcbrun |
|
I'm not sure why the checks are stuck. @bstoll for any insight? |
|
I'll merge this today after all checks pass |
|
@dplore @robshakir I created this PR to Felix's fork to generate the new test files and make the code generation work from macOS. I also added some documentation. Let me know what you think: felix-kaestner#1 |
|
@dplore Please do not merge this directly. I would like to review the code. |
Got it. I saw your earlier approval #157 (review) so I thought your review was complete. |
ACK, thanks -- the question above about backwards compatibility was what I wanted to respond to -- I missed it in my original review :-) Thanks. |
I'd like to keep backwards compatibility, I'd missed that this would result in a change (and it seems like the tests didn't catch this either). It looks like the docstring is a bit inconsistent here ( @felix-kaestner Are you OK to make this change please? |
|
@felix-kaestner please let us know if you can make the changes @robshakir has suggested. If not, perhaps @eoswald could take it up? |
felix-kaestner
force-pushed
the
main
branch
from
68dcb70 to
0c32405
Compare
Hey @dplore, sorry for the delayed response, I was quite busy the last couple of days. I just updated the PR with the suggested changes, renaming
@robshakir The changes do not alter the default behavior, thus keeping backwards compatibility. It's just an additional option the allows to opt-out of prepending the YANG module name to fields. |
robshakir
left a comment
robshakir
left a comment
There was a problem hiding this comment.
Thank you for the changes and clarifications - much appreciated. Merging this code. Thanks very much for the contribution!
As openconfig/ygnmi#157 got merged, we no longer have use our fork for making adjustments to the marshalling performed by ygnmi but can now use the latest upstream version instead.
As openconfig/ygnmi#157 got merged, we no longer have use our fork for making adjustments to the marshalling performed by ygnmi but can now use the latest upstream version instead.
5 participants
This patch extends the current behavior to populate the gnmi set request in order to allow more control over the request payload.
WithAppendModuleNameoption is added that allows to toggle the procedure of adding the YANG module name to the respective fields, which is redundant for systems that will automatically infer the module name via other means.WithEncodingoption is respected when set to json encoding. Previously the typed value was marshalled by default as json_ietf, while certain platform such as Cisco NX-OS only support json enconding and therefore expect all request to be encoded as such. With this option, it's now possible to encode request to json if desired.All changes mentioned above do not alter the current behavior. They simply allow for more flexibility when used explicitly by providing the respective options.