-
Notifications
You must be signed in to change notification settings - Fork 184
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
Feat(eos_cli_config_gen): Add support for grpc-tunnel #2696
Feat(eos_cli_config_gen): Add support for grpc-tunnel #2696
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this PR! Very complete implementation.
I have added a couple of comments mostly around adding constraints in the schema, that will make for simpler jinja templates and cleaning up a bit of extra lines in the documentation.
Do not hesitate to ask questions / precisions
For reference:
Leaf1(config-grpc-tunnel-transport-BLAH)#show cli commands
APPEND [ COMMENT ]
[no|default] comment
[no|default] destination HOST port PORT
do CMD
exit
[no|default] gnmi ssl profile PROFILE
[no|default] local interface INTF [ port PORT ]
show active
show active all [ detail ]
show comment
[no|default] shutdown
[no|default] target ( serial-number | TARGET_ID )
[no|default] tunnel ssl profile PROFILE
[no|default] vrf VRF
.../arista/avd/roles/eos_cli_config_gen/schemas/schema_fragments/management_api_gnmi.schema.yml
Outdated
Show resolved
Hide resolved
.../arista/avd/roles/eos_cli_config_gen/schemas/schema_fragments/management_api_gnmi.schema.yml
Outdated
Show resolved
Hide resolved
.../arista/avd/roles/eos_cli_config_gen/schemas/schema_fragments/management_api_gnmi.schema.yml
Outdated
Show resolved
Hide resolved
.../arista/avd/roles/eos_cli_config_gen/schemas/schema_fragments/management_api_gnmi.schema.yml
Outdated
Show resolved
Hide resolved
ansible_collections/arista/avd/roles/eos_cli_config_gen/templates/eos/management-api-gnmi.j2
Outdated
Show resolved
Hide resolved
.../arista/avd/roles/eos_cli_config_gen/schemas/schema_fragments/management_api_gnmi.schema.yml
Show resolved
Hide resolved
...s/arista/avd/molecule/eos_cli_config_gen/inventory/host_vars/management-gnmi-grpc-tunnel.yml
Outdated
Show resolved
Hide resolved
..._collections/arista/avd/molecule/eos_cli_config_gen/documentation/devices/management-gnmi.md
Outdated
Show resolved
Hide resolved
...llections/arista/avd/roles/eos_cli_config_gen/templates/documentation/management-api-gnmi.j2
Outdated
Show resolved
Hide resolved
.../arista/avd/molecule/eos_cli_config_gen/documentation/devices/management-gnmi-grpc-tunnel.md
Outdated
Show resolved
Hide resolved
.../arista/avd/molecule/eos_cli_config_gen/documentation/devices/management-gnmi-grpc-tunnel.md
Show resolved
Hide resolved
I appreciate the quick review! I added changes in-line with what was requested. Took a bit to work out the whitespace details, but I think I got it this time. |
You left quite a few comments so GitHub hid some of them. I think they are all covered now :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fast update - a couple new comments regarding the remaining extra blank line and the useless check for name
..._collections/arista/avd/molecule/eos_cli_config_gen/documentation/devices/management-gnmi.md
Outdated
Show resolved
Hide resolved
...llections/arista/avd/roles/eos_cli_config_gen/templates/documentation/management-api-gnmi.j2
Outdated
Show resolved
Hide resolved
{% if transport.name is arista.avd.defined %} | ||
transport grpc-tunnel {{ transport.name }} | ||
{% endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this one still needs to be addressed!
...llections/arista/avd/roles/eos_cli_config_gen/templates/documentation/management-api-gnmi.j2
Outdated
Show resolved
Hide resolved
...llections/arista/avd/roles/eos_cli_config_gen/templates/documentation/management-api-gnmi.j2
Outdated
Show resolved
Hide resolved
..._collections/arista/avd/molecule/eos_cli_config_gen/documentation/devices/management-gnmi.md
Outdated
Show resolved
Hide resolved
.../arista/avd/molecule/eos_cli_config_gen/documentation/devices/management-gnmi-grpc-tunnel.md
Show resolved
Hide resolved
I appreciate the patience and continual feedback! It appears, to my eye, that the spacing is fixed on the documentation. |
..._collections/arista/avd/molecule/eos_cli_config_gen/documentation/devices/management-gnmi.md
Outdated
Show resolved
Hide resolved
...ns/arista/avd/molecule/eos_cli_config_gen/documentation/devices/management-gnmi-new-flags.md
Outdated
Show resolved
Hide resolved
.../arista/avd/molecule/eos_cli_config_gen/documentation/devices/management-gnmi-grpc-tunnel.md
Outdated
Show resolved
Hide resolved
port: 10001 | ||
target: | ||
use_serial_number: true | ||
target_id: testid1 testid2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be better as a list that we then join on white space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- +1, this should be a list and renamed to
target_ids
.../arista/avd/roles/eos_cli_config_gen/schemas/schema_fragments/management_api_gnmi.schema.yml
Outdated
Show resolved
Hide resolved
.../arista/avd/roles/eos_cli_config_gen/schemas/schema_fragments/management_api_gnmi.schema.yml
Outdated
Show resolved
Hide resolved
@adietrich-ussignal - would you have some time to look at the latest review comments on this PR - we would love to include it in the next dev release or the one just after! Let us know if you need some help on addressing them! |
I'll review the comments and push commits on this issue tomorrow. |
…s/schema_fragments/management_api_gnmi.schema.yml Co-authored-by: Guillaume Mulocher <gmulocher@arista.com>
Co-authored-by: Guillaume Mulocher <gmulocher@arista.com>
Co-authored-by: Carl Buchmann <carl.buchmann@arista.com>
Co-authored-by: Carl Buchmann <carl.buchmann@arista.com>
Having some issues getting the proper commits amidst the other changes. I would greatly appreciate some guidance to resolve. |
76fddef
to
d2cfa5f
Compare
Hi @adietrich-ussignal - fixed the things going on with schema. |
Thanks @gmuloc! Learned a lot through this one and by reviewing your latest code revision. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for this great contribution @adietrich-ussignal
Change Summary
Add grpc-tunnel to the management api gnmi data model.
Related Issue(s)
Fixes #2665
Component(s) name
arista.avd.eos_cli_config_gen
Proposed changes
This change adds the grpc-tunnel configuration settings to the management api gnmi data model. Allows for the creation of multiple tunnels, and multiple target_ids per tunnel as is permitted by EOS. Multiple target_ids can be separated by a space.
This configuration is only valid as of EOS 4.27.0F.
Added the following to schema management_api_gnmi
How to test
Ran the pre-commit on changes to the schema and Jinja2 templates for both EOS and Documentation.
Added tests to the eos_cli_config_gen molecule using management-gnmi-grpc-tunnel.yml.
Checklist
User Checklist
Repository Checklist