Conversation
Summary of ChangesHello @manan-patel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a fix for gNMI 1.6, specifically targeting compatibility and testing stability with Cisco devices within the gNMI network instance tests. The changes involve adjusting the test logic to bypass gNMI server configuration for Cisco platforms and removing a previously defined interface deviation, which collectively aim to resolve issues encountered on these devices. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix an issue with gNMI on Cisco devices by conditionally skipping the creation of a gNMI server on the default network instance. While the intent is clear, the implementation introduces a hardcoded vendor check. My review focuses on refactoring this logic to use the existing deviations framework in the project, which will improve maintainability and adherence to the established architecture.
| if dut.Vendor() != ondatra.CISCO { | ||
| cfgplugins.CreateGNMIServer(batch, t, dut, &dutPort1NetworkInstanceIParams) | ||
| } |
There was a problem hiding this comment.
This hardcoded vendor check for ondatra.CISCO should be implemented as a deviation to align with the project's established pattern for handling vendor-specific behavior. Using the deviations framework improves maintainability and makes it easier to manage exceptions for different platforms.
Please consider adding a new deviation, for example skip_gnmi_server_on_default_ni, and a corresponding function SkipGNMIServerOnDefaultNI(dut *ondatra.DUTDevice) bool in the internal/deviations package.
The code here would then become:
if !deviations.SkipGNMIServerOnDefaultNI(dut) {
cfgplugins.CreateGNMIServer(batch, t, dut, &dutPort1NetworkInstanceIParams)
}This would also require adding the deviation to the metadata.textproto for the CISCO platform.
Pull Request Test Coverage Report for Build 19554444493Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
| } | ||
| deviations: { | ||
| interface_enabled: true | ||
| skip_grpc_validation_in_default_vrf: true |
There was a problem hiding this comment.
Skip grpc validation in default? Seems more like we are tring to skip the configuration.
CreateGNMIServer uses batchUpdate which in OC, this should not change between old and new grpc sever syntax.
|
This deviation is skipping configuring grpc server in default vrf; this should not be the case. This should not be the case, if the grpc server is already present (under assumption bootstrap config has it pre-preconfigured) and we are are latter configuring it back, the OC config should not impact the server. Can we include in this PR changing the base config ? Also, Are other FNT broken when the base config is changed? |
|
/fptest cisco-8000e |
danielbarney
left a comment
danielbarney
left a comment
There was a problem hiding this comment.
Add base config changes in which grpc server is configured in default and latter with batchUpdate we add one for "customVrf"
google3/ops/netops/lab/kne/guitar/functional_tests/topologies/cisco_8000e_config.cfg
danielbarney
left a comment
danielbarney
left a comment
There was a problem hiding this comment.
I will close this pull request and submit a new one with modifications based on bootstrap config test
4 participants
No description provided.