ZSTAC-86553 Fix IPv6 NBD target URI formatting#4412
Merged
Conversation
Format IPv6 hosts with brackets when building NBD target URIs. ZSTAC-86553 Test: mvn -pl header -DskipTests compile Change-Id: I60ec48719c304cddbeb06954050ac2fe
Walkthrough本次改动为 addon 和 backup 两个包中的 NbdRemoteTarget 类引入 IPv6NetworkUtils.formatHostForUrl，在生成 NBD 目标 URI 时对主机部分进行格式化，使 IPv6 地址正确加上中括号。新增测试用例验证该行为。 ChangesNBD URI IPv6 主机格式化
Estimated code review effort: 1 (Trivial) | ~5 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant NbdRemoteTarget
participant IPv6NetworkUtils
Caller->>NbdRemoteTarget: getResourceURI()/getTargetUri(hostname)
NbdRemoteTarget->>IPv6NetworkUtils: formatHostForUrl(ip/hostname)
IPv6NetworkUtils-->>NbdRemoteTarget: 格式化后的host
NbdRemoteTarget-->>Caller: nbd:// URI
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/src/test/groovy/org/zstack/test/integration/core/ManagementNetworkIpv6Case.groovy (1)
92-130: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win新增测试方法未被
test()调用,实际不会执行。
testNbdTargetUriUsesBracketedIpv6Host()(第265-275行)定义完整,但在唯一的@Test入口test()方法的调用列表(93-129行)中未找到对它的调用。该测试虽然能编译通过,但永远不会被执行,PR 所声称的"新增测试覆盖"实际未生效。🔧 修复建议:在合适位置补充调用
testSshTargetUsesRawIpv6Host() testScpTargetUsesBracketedIpv6Host() + testNbdTargetUriUsesBracketedIpv6Host() testCallbackCheckerUsesIpv6Options()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/src/test/groovy/org/zstack/test/integration/core/ManagementNetworkIpv6Case.groovy` around lines 92 - 130, The new test method testNbdTargetUriUsesBracketedIpv6Host() is defined but never executed because it is missing from the test() invocation list. Update the ManagementNetworkIpv6Case.groovy test() method to call testNbdTargetUriUsesBracketedIpv6Host() alongside the other test* methods so the added coverage actually runs.
🧹 Nitpick comments (1)
test/src/test/groovy/org/zstack/test/integration/core/ManagementNetworkIpv6Case.groovy (1)
270-273: 📐 Maintainability & Code Quality | 🔵 Trivial建议补充 addon 场景的 IPv4 断言以保持对称性。
backupTarget同时验证了 IPv6 与 IPv4 两种情形,而addonTarget只验证了 IPv6。建议补充一条 IPv4 断言,确保addon.NbdRemoteTarget在两种地址族下都有覆盖。♻️ 建议补充的断言
def addonTarget = new org.zstack.header.storage.addon.NbdRemoteTarget() addonTarget.setIp(IPV6) addonTarget.setPort(10401) assert addonTarget.getResourceURI() == "nbd://[2001:db8::1]:10401" + + addonTarget.setIp(IPV4) + assert addonTarget.getResourceURI() == "nbd://192.168.1.10:10401"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/src/test/groovy/org/zstack/test/integration/core/ManagementNetworkIpv6Case.groovy` around lines 270 - 273, The addon.NbdRemoteTarget test currently covers only the IPv6 getResourceURI case, so add a matching IPv4 assertion to keep parity with backupTarget coverage. Update the ManagementNetworkIpv6Case test around addonTarget to also set an IPv4 address and verify the URI format, using the existing NbdRemoteTarget methods setIp and getResourceURI as the anchors for the new assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@test/src/test/groovy/org/zstack/test/integration/core/ManagementNetworkIpv6Case.groovy`:
- Around line 92-130: The new test method
testNbdTargetUriUsesBracketedIpv6Host() is defined but never executed because it
is missing from the test() invocation list. Update the
ManagementNetworkIpv6Case.groovy test() method to call
testNbdTargetUriUsesBracketedIpv6Host() alongside the other test* methods so the
added coverage actually runs.
---
Nitpick comments:
In
`@test/src/test/groovy/org/zstack/test/integration/core/ManagementNetworkIpv6Case.groovy`:
- Around line 270-273: The addon.NbdRemoteTarget test currently covers only the
IPv6 getResourceURI case, so add a matching IPv4 assertion to keep parity with
backupTarget coverage. Update the ManagementNetworkIpv6Case test around
addonTarget to also set an IPv4 address and verify the URI format, using the
existing NbdRemoteTarget methods setIp and getResourceURI as the anchors for the
new assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: e69b091c-f983-4f22-b3fe-3c48469b8f89
📒 Files selected for processing (3)
header/src/main/java/org/zstack/header/storage/addon/NbdRemoteTarget.javaheader/src/main/java/org/zstack/header/storage/backup/NbdRemoteTarget.javatest/src/test/groovy/org/zstack/test/integration/core/ManagementNetworkIpv6Case.groovy
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
mvn -pl header -DskipTests compilemvn -f test/pom.xml -Dtest=org.zstack.test.integration.core.ManagementNetworkIpv6Case testwas attempted, but the local test module fails before this case due existing KVMHostUtils/ApiResponse API mismatches.Jira: ZSTAC-86553
sync from gitlab !10373