feat(deploy): 添加自定义 Maven 命令功能 #17
Conversation
- 新增 mvnCmd 字符串变量，用于存储自定义 Maven 命令 - 在帮助文档中添加 Scenario 1，说明如何使用自定义 Maven 命令进行构建和部署 - 修改 execMavenBuild 函数，支持使用自定义 Maven 命令 - 在 init 函数中添加 mvnCmd 的命令行参数定义
WalkthroughAdds a package-level Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "deploy CLI (deploy.go)"
participant OS as "OS Executor"
Note over User,CLI: User invokes deploy (optional --mvnCmd)
User->>CLI: deploy [--mvnCmd "<cmd ...>"]
CLI->>CLI: parse flags -> mvnCmd
CLI->>CLI: parseMavenCommand(mvnCmd) -> (compileCmd, compileArgs)
CLI->>CLI: assemble full build command (compileCmd + compileArgs + extra args)
CLI->>OS: execute build command
alt build succeeds
OS-->>CLI: exit 0
CLI-->>User: proceed with deployment
else build fails
OS-->>CLI: non-zero exit
CLI-->>User: report build error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
v1/cmd/deploy/deploy.go (2)
76-78: Improve the example clarity and fix grammatical issues.The new scenario example and description need refinement:
- The example uses
${mvnCmd}which might confuse users - they should provide an actual Maven command- There's a grammatical error and redundant text in the description
Apply this diff to improve the example:
-Scenario 1: Build the bundle using a custom Maven command to compile the project in the current workspace and deploy it to a locally running Ark container on the default port.: - arkctl deploy --mvnCmd ${mvnCmd} +Scenario 1: Build the bundle using a custom Maven command in the current workspace and deploy it to a locally running Ark container: + arkctl deploy --mvnCmd "mvn clean compile package -DskipTests"
468-470: Fix typo in flag description.There's a grammatical error in the flag description that makes it unclear.
Apply this diff to fix the description:
DeployCommand.Flags().StringVar(&mvnCmd, "mvnCmd", "", ` -If Provided, arkctl will try to deploy the mvnCmd to compile maven project. +If Provided, arkctl will use the mvnCmd to compile the maven project. `)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v1/cmd/deploy/deploy.go(4 hunks)
🔇 Additional comments (1)
v1/cmd/deploy/deploy.go (1)
55-55: LGTM!The global variable declaration is clean, well-commented, and follows the existing code patterns.
- 修复了自定义 Maven 命令的参数解析逻辑 - 现在可以正确处理空格分隔的 Maven 命令参数
|
@ccwxl 请补充相应的单测。 |
- 将 Maven命令解析逻辑提取到单独的函数 parseMavenCommand 中- 在 deploy_test.go 中添加 parseMavenCommand 函数的单元测试 - 优化了命令行参数的分割方式，使用 strings.Fields替代 strings.Split
fix |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
v1/cmd/deploy/deploy_test.go (5)
14-14: Testing an unexported helper within the same package is fine; consider adding one higher-level test.Keeping this test in package deploy is OK for validating parseMavenCommand directly. As a complement (not a replacement), consider one integration-style test that exercises the flag/exec path (e.g., verifying the constructed exec.Command passed by execMavenBuild via a small seam or test double). This guards against regressions in wiring without overexposing internals.
If you want, I can sketch a minimal seam around execMavenBuild to capture the command/args without spawning a process.
16-19: Use a diff-based assertion for clearer failures.reflect.DeepEqual works, but failures are opaque. Using go-cmp produces precise, readable diffs, speeding up triage. Also quote the cmd values in the error for clarity.
Apply this diff:
@@ -import ( - "reflect" - "testing" -) +import ( + "testing" + "github.com/google/go-cmp/cmp" +) @@ - if gotCmd != tt.wantCmd { - t.Errorf("parseMavenCommand() gotCmd = %v, want %v", gotCmd, tt.wantCmd) - } - if !reflect.DeepEqual(gotArgs, tt.wantArgs) { - t.Errorf("parseMavenCommand() gotArgs = %v, want %v", gotArgs, tt.wantArgs) - } + if gotCmd != tt.wantCmd { + t.Errorf("parseMavenCommand() gotCmd = %q, want %q", gotCmd, tt.wantCmd) + } + if diff := cmp.Diff(tt.wantArgs, gotArgs); diff != "" { + t.Errorf("parseMavenCommand() args mismatch (-want +got):\n%s", diff) + }If you prefer to avoid a new test dependency, keep reflect.DeepEqual but still switch to %q for quoting the string values.
Also applies to: 63-68
28-58: Broaden table cases to cover whitespace and common path variants.Current cases are solid. Adding a few more helps lock behavior around trimming, tabs/newlines, and typical mvnw(.cmd) paths.
Apply this diff to extend the table:
@@ { name: "command with multiple spaces", mvnCmd: "mvn clean package", wantCmd: "mvn", wantArgs: []string{"clean", "package"}, }, + { + name: "whitespace-only command", + mvnCmd: " \t\n ", + wantCmd: "mvn", + wantArgs: []string{"clean", "package", "-Dmaven.test.skip=true"}, + }, + { + name: "leading and trailing spaces", + mvnCmd: " mvn clean package ", + wantCmd: "mvn", + wantArgs: []string{"clean", "package"}, + }, + { + name: "tabs between tokens", + mvnCmd: "mvn\tclean\tinstall", + wantCmd: "mvn", + wantArgs: []string{"clean", "install"}, + }, + { + name: "windows wrapper with flags", + mvnCmd: "mvnw.cmd -q -T1C", + wantCmd: "mvnw.cmd", + wantArgs: []string{"-q", "-T1C"}, + }, + { + name: "absolute path mvn", + mvnCmd: "/usr/local/bin/mvn -q", + wantCmd: "/usr/local/bin/mvn", + wantArgs: []string{"-q"}, + }, + { + name: "relative path wrapper", + mvnCmd: "./mvnw -q", + wantCmd: "./mvnw", + wantArgs: []string{"-q"}, + }, + { + name: "args with equals and profile", + mvnCmd: "mvn -DskipTests=true -Pprod", + wantCmd: "mvn", + wantArgs: []string{"-DskipTests=true", "-Pprod"}, + },Note: If parseMavenCommand intentionally does simple whitespace tokenization (e.g., strings.Fields), we should explicitly document that quoted arguments with spaces are not supported via the single --mvnCmd flag. If you plan to support shell-like quoting, we can switch the parser and add corresponding tests.
21-21: Make tests parallel-safe for faster runs.Subtests here don’t share mutable state; enabling parallelism can speed CI on larger suites.
Apply this diff:
@@ func TestParseMavenCommand(t *testing.T) { + t.Parallel() @@ - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() gotCmd, gotArgs := parseMavenCommand(tt.mvnCmd) if gotCmd != tt.wantCmd { t.Errorf("parseMavenCommand() gotCmd = %v, want %v", gotCmd, tt.wantCmd) } - if !reflect.DeepEqual(gotArgs, tt.wantArgs) { - t.Errorf("parseMavenCommand() gotArgs = %v, want %v", gotArgs, tt.wantArgs) - } + if !reflect.DeepEqual(gotArgs, tt.wantArgs) { + t.Errorf("parseMavenCommand() gotArgs = %v, want %v", gotArgs, tt.wantArgs) + } }) }Also applies to: 61-69
28-33: Avoid hard-coding defaults to reduce drift risk.The default args ["clean","package","-Dmaven.test.skip=true"] are duplicated here. If these defaults are also defined in deploy.go, consider exposing them (e.g., as unexported consts in a test-only helper or exported for reuse) to keep tests in sync with the source of truth.
If you want, I can propose a small refactor to centralize these defaults and update both deploy.go and this test accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
v1/cmd/deploy/deploy.go(5 hunks)v1/cmd/deploy/deploy_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v1/cmd/deploy/deploy.go
🔇 Additional comments (1)
v1/cmd/deploy/deploy_test.go (1)
1-13: License header looks good.Apache-2.0 header is complete and correctly formatted.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
v1/cmd/deploy/deploy_test.go (3)
25-25: Use slices.Clone for clarity when copying default args.Functionally equivalent, slightly clearer in Go 1.21+.
Apply:
-import ( - "testing" - - "github.com/google/go-cmp/cmp" -) +import ( + "slices" + "testing" + + "github.com/google/go-cmp/cmp" +) @@ - defaultArgs := append([]string(nil), defaultMavenArgs...) + defaultArgs := slices.Clone(defaultMavenArgs)
107-119: Assertion strategy is solid; consider tolerating nil vs empty slices if parser returns nil.Not required, but reduces brittleness if parseMavenCommand returns nil for zero args.
Apply:
-import ( - "testing" - - "github.com/google/go-cmp/cmp" -) +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" +) @@ - if diff := cmp.Diff(tt.wantArgs, gotArgs); diff != "" { + if diff := cmp.Diff(tt.wantArgs, gotArgs, cmpopts.EquateEmpty()); diff != "" { t.Errorf("parseMavenCommand() args mismatch (-want +got):\n%s", diff) }
27-105: Add non-quoted token tests & clarify default-args overrideI’ve confirmed that
parseMavenCommandusesstrings.Fieldsfor splitting (whitespace-only tokenization, no shell-style quoting) and that supplying any--mvnCmd—even just “mvn” or “mvnw”—overrides the default args (tests at v1/cmd/deploy/deploy.go around lines 168 & 480). To improve coverage and prevent surprises, I recommend:• Add test cases exercising separate-value flags and multi-value tokens:
• “thread-count with separate value” (mvn -T 1C→ args[-T,1C])
• “multiple profiles in one flag” (mvn -Pprod,dev→ args[-Pprod,dev])
• “reactor and project list” (mvn -am -pl :module→ args[-am,-pl,:module])• Update the
--mvnCmdflag help (around line 478 in deploy.go) to note that providing any custom command disables the default args, so callers aren’t surprised by zero args when they pass just the command.Let me know if you’d like me to open a follow-up for tests + a doc blurb!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(2 hunks)v1/cmd/deploy/deploy.go(5 hunks)v1/cmd/deploy/deploy_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v1/cmd/deploy/deploy.go
🔇 Additional comments (4)
go.mod (1)
9-9: Adding go-cmp as a direct dependency is appropriate for test usage.No issues; aligns with the new table-driven tests using cmp.
v1/cmd/deploy/deploy_test.go (3)
1-13: License header present and correct.Looks good.
14-14: Testing in the same package enables coverage of unexported parseMavenCommand.Good choice to validate internal parsing behavior without exporting it.
22-24: Parallelization is correctly applied at both parent and subtest levels.Nice use of t.Parallel with subtest variable shadowing to avoid capture issues.
| github.com/spf13/viper v1.10.1 | ||
| github.com/stretchr/testify v1.8.4 | ||
| golang.org/x/net v0.23.0 | ||
| golang.org/x/text v0.14.0 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm golang.org/x/text version and security posture.
You moved x/text to a direct dependency at v0.14.0. Please verify if a newer patch/minor is available and whether any advisories apply.
Run:
🏁 Script executed:
#!/bin/bash
# Check where x/text is used
rg -n 'golang.org/x/text' -C2
# Check available updates (requires Go toolchain)
go list -m -u -json golang.org/x/text | jq '.Version, .Update'
# Optional: vuln check (if govulncheck installed)
govulncheck golang.org/x/text@v0.14.0Length of output: 2404
Update x/text dependency to the latest release and verify security posture
- go.mod (line 19): bump golang.org/x/text from v0.14.0 to v0.28.0 (released 2025-08-07)
- v1/cmd/create/create.go: ensure the import of “golang.org/x/text/encoding/simplifiedchinese” still resolves correctly
- After updating, run a vulnerability scan (e.g. install/run
govulncheck golang.org/x/text@v0.28.0orgo list -m -u -json golang.org/x/text@v0.28.0) to confirm no advisories apply
Proposed diff:
--- a/go.mod
+++ b/go.mod
@@ -16,7 +16,7 @@ require (
github.com/stretchr/testify v1.8.4
golang.org/x/net v0.23.0
- golang.org/x/text v0.14.0
+ golang.org/x/text v0.28.0
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| golang.org/x/text v0.14.0 | |
| require ( | |
| github.com/stretchr/testify v1.8.4 | |
| golang.org/x/net v0.23.0 | |
| golang.org/x/text v0.28.0 | |
| ) |
2 participants
Summary by CodeRabbit