gNMI-3 : Updated changes to support Juniper devices#5618
gNMI-3 : Updated changes to support Juniper devices#5618priyaranjannanda wants to merge 2 commits into
Conversation
priyaranjannanda
commented
Jun 25, 2026
priyaranjannanda
commented
- Changes to o support Juniper devices
Summary of ChangesHello, 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 introduces necessary adjustments to the gNMI union_replace test suite to ensure compatibility with Juniper network devices. The changes primarily focus on handling Junos-specific configuration hierarchies, managing mandatory YANG statements, and addressing pre-commit conflicts that arise when CLI and OpenConfig operations overlap under config-namespace mode. 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 the 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 counterproductive. 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. Footnotes
|
Pull Request Functional Test Report for #5618 / ae3c0e2Virtual Devices
Hardware Devices
|
![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 adapts the union replace tests to support Juniper devices under config-namespace mode. It introduces a helper to strip top-level Junos configuration blocks, adds mandatory interface type statements, and skips CLI overrides that conflict with OpenConfig-managed paths on Juniper. Feedback focuses on improving the block-stripping logic to avoid matching nested blocks, replacing direct vendor checks (dut.Vendor()) with the deviations package while explicitly skipping unsupported test paths, and removing unreachable dead code in TestUnionReplace3_6_1.
| trimmed := strings.TrimSpace(line) | ||
| if !inStripped && depth == 0 { | ||
| // Check if this line opens one of the blocks to strip. | ||
| for blockName := range stripped { | ||
| if trimmed == blockName+" {" { | ||
| inStripped = true | ||
| depth = 1 | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
Using strings.TrimSpace(line) before checking if the line starts a block to strip can lead to matching nested blocks (e.g., a nested groups block inside a BGP configuration) rather than only top-level blocks. Since top-level blocks in Junos configuration never have leading whitespace, we should enforce that there is no leading whitespace by using strings.TrimRight instead of strings.TrimSpace.
| trimmed := strings.TrimSpace(line) | |
| if !inStripped && depth == 0 { | |
| // Check if this line opens one of the blocks to strip. | |
| for blockName := range stripped { | |
| if trimmed == blockName+" {" { | |
| inStripped = true | |
| depth = 1 | |
| break | |
| } | |
| } | |
| if !inStripped && depth == 0 { | |
| // Check if this line opens one of the blocks to strip. | |
| // Top-level blocks in Junos configuration have no leading whitespace. | |
| for blockName := range stripped { | |
| if strings.TrimRight(line, " \r\t") == blockName+" {" { | |
| inStripped = true | |
| depth = 1 | |
| break | |
| } | |
| } |
| } else { | ||
| t.Logf("Skipping CLI mtu override for Juniper: CLI interface mtu conflicts with OC-managed mtu under config-namespace mode.") | ||
| } |
There was a problem hiding this comment.
Silently bypassing the core action of a test case (the CLI MTU override) and letting the test pass without executing its assertions is misleading. If a test case or a portion of it cannot be run on a specific platform, it should be explicitly skipped using t.Skip. Additionally, avoid using dut.Vendor() for vendor-specific logic; instead, use the deviations package to maintain test abstraction and portability.
if deviations.CliMtuOverrideUnsupported(dut) {
t.Skip("Skipping CLI mtu override: CLI interface mtu conflicts with OC-managed mtu under config-namespace mode.")
}References
- Avoid using "dut.Vendor()" for vendor-specific logic or configurations in tests. Instead, use the "deviations" package to maintain test abstraction and portability across different vendors.
| } else { | ||
| t.Logf("Skipping CLI description override for Juniper: CLI interface description conflicts with OC-managed description under config-namespace mode.") | ||
| } |
There was a problem hiding this comment.
Similar to the MTU override test, silently bypassing the CLI description override and letting the test pass without validation is misleading. Use t.Skip to explicitly skip this portion of the test. Ensure you use a deviation check from the deviations package instead of checking dut.Vendor() directly to maintain test portability.
if deviations.CliDescriptionOverrideUnsupported(dut) {
t.Skip("Skipping CLI description override: CLI interface description conflicts with OC-managed description under config-namespace mode.")
}References
- Avoid using "dut.Vendor()" for vendor-specific logic or configurations in tests. Instead, use the "deviations" package to maintain test abstraction and portability across different vendors.
| if dut.Vendor() == ondatra.JUNIPER { | ||
| speed := gnmi.Get(t, dut, gnmi.OC().Interface(dp1.Name()).Ethernet().PortSpeed().State()) | ||
| t.Logf("PortSpeed state after union_replace: %v", speed) | ||
| } else { | ||
| gnmi.Watch(t, dut, gnmi.OC().Interface(dp1.Name()).Ethernet().PortSpeed().Config(), awaitTimeOut, func(val *ygnmi.Value[oc.E_IfEthernet_ETHERNET_SPEED]) bool { | ||
| speed, present := val.Val() | ||
| if !present { | ||
| t.Logf("PortSpeed config not present. Want: %v, got: not present", targetSpeed) | ||
| return false | ||
| } | ||
| if speed != targetSpeed { | ||
| t.Logf("PortSpeed config not set to target speed. Want: %v, got: %v", targetSpeed, speed) | ||
| return false | ||
| } | ||
| t.Logf("PortSpeed config is set to target speed: %v", speed) | ||
| return true | ||
| }).Await(t) | ||
| } |
There was a problem hiding this comment.
Since TestUnionReplace3_6_1 is skipped at the very beginning for Juniper (line 560), the check if dut.Vendor() == ondatra.JUNIPER here is unreachable dead code. Removing this conditional block and directly using the gnmi.Watch call also aligns with avoiding direct vendor checks in favor of portability.
gnmi.Watch(t, dut, gnmi.OC().Interface(dp1.Name()).Ethernet().PortSpeed().Config(), awaitTimeOut, func(val *ygnmi.Value[oc.E_IfEthernet_ETHERNET_SPEED]) bool {
speed, present := val.Val()
if !present {
t.Logf("PortSpeed config not present. Want: %v, got: not present", targetSpeed)
return false
}
if speed != targetSpeed {
t.Logf("PortSpeed config not set to target speed. Want: %v, got: %v", targetSpeed, speed)
return false
}
t.Logf("PortSpeed config is set to target speed: %v", speed)
return true
}).Await(t)References
- Avoid using "dut.Vendor()" for vendor-specific logic or configurations in tests. Instead, use the "deviations" package to maintain test abstraction and portability across different vendors.
2 participants
