feat: Allow using kubectl built-in kustomize when separate kustomize binary is missing#173
feat: Allow using kubectl built-in kustomize when separate kustomize binary is missing#173
Conversation
…binary is missing Implements #63: Automatically fallback to kubectl's built-in kustomize when the standalone kustomize binary is not available. Changes: - Modified kustomizeBin() to detect and use kubectl kustomize as fallback - Fixed runBytes() to properly handle commands with subcommands like 'kubectl kustomize' - Added isUsingKubectlKustomize() helper to detect when using kubectl kustomize - Updated kustomizeEnableAlphaPluginsFlag() and kustomizeLoadRestrictionsNoneFlag() to skip version detection when using kubectl kustomize - Added clear error messages for unsupported features (edit subcommands) when using kubectl kustomize - Modified KustomizeBuild() to omit 'build' argument when using kubectl kustomize - Modified patch.go to omit 'build' argument and tempDir when using kubectl kustomize - Added TestKustomizeBin test to verify fallback behavior - Added TestKubectlKustomizeFallback integration test Signed-off-by: yxxhero <aiopsclub@163.com>
There was a problem hiding this comment.
Pull request overview
Adds an automatic fallback to kubectl’s built-in kustomize implementation when a standalone kustomize binary is not available, and updates command execution to support multi-word “commands” like kubectl kustomize.
Changes:
- Update
Runner.kustomizeBin()to preferkustomizeif present, otherwise fall back tokubectl kustomize. - Fix
Runner.runBytes()to correctly handle commands with subcommands embedded in thecmdstring. - Adjust kustomize build/patch flows and add tests around the new behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| util_test.go | Adds unit tests for kustomizeBin() selection logic (option, PATH lookup, fallback). |
| runner.go | Implements the fallback selection in kustomizeBin() and improves command parsing for multi-word commands. |
| patch.go | Updates patch rendering to support kubectl-kustomize mode (argument handling changes). |
| kustomize.go | Adds kubectl-kustomize detection, skips version probing, and adjusts build/edit behavior accordingly. |
| kubectl_kustomize_test.go | Adds a test intended to validate kubectl kustomize support. |
Comments suppressed due to low confidence (1)
patch.go:201
- When using
kubectl kustomize, this invocation omits the kustomization directory argument and also runs withdir==""(current working directory). That meanskubectl kustomizewill not read the generated kustomization.yaml undertempDir, so patch rendering will fail. PasstempDiras the final arg for the kubectl case (similar toKustomizeBuild), or switch torunInDir(tempDir, ...)and rely on the default.target.
kustomizeArgs := []string{"--output", renderedFile}
if !r.isUsingKubectlKustomize() {
kustomizeArgs = append([]string{"build", tempDir}, kustomizeArgs...)
}
if u.EnableAlphaPlugins {
f, err := r.kustomizeEnableAlphaPluginsFlag()
if err != nil {
return err
}
kustomizeArgs = append(kustomizeArgs, f)
}
_, err := r.run(nil, r.kustomizeBin(), kustomizeArgs...)
if err != nil {
| t.Run("KUSTOMIZE_BIN environment variable", func(t *testing.T) { | ||
| if _, err := exec.LookPath("kustomize"); err != nil { | ||
| t.Skip("kustomize binary not found in PATH") | ||
| } | ||
| if _, ok := os.LookupEnv("KUSTOMIZE_BIN"); ok { | ||
| t.Skip("KUSTOMIZE_BIN environment variable is already set") | ||
| } | ||
| os.Setenv("KUSTOMIZE_BIN", "/custom/kustomize") | ||
| defer os.Unsetenv("KUSTOMIZE_BIN") | ||
| r := New(KustomizeBin(os.Getenv("KUSTOMIZE_BIN"))) | ||
| got := r.kustomizeBin() | ||
| want := "/custom/kustomize" | ||
| if got != want { | ||
| t.Errorf("kustomizeBin() = %v, want %v", got, want) | ||
| } | ||
| }) |
There was a problem hiding this comment.
This subtest name suggests that kustomizeBin() reads KUSTOMIZE_BIN directly, but the test actually passes the env var value via KustomizeBin(...) (same behavior as the first subtest). Rename the subtest to reflect that it’s testing the option value, or update kustomizeBin() to consult KUSTOMIZE_BIN if that’s intended.
| t.Run("kustomize binary exists in PATH", func(t *testing.T) { | ||
| if _, err := exec.LookPath("kustomize"); err != nil { | ||
| t.Skip("kustomize binary not found in PATH") | ||
| } | ||
| r := New() | ||
| got := r.kustomizeBin() | ||
| want := "kustomize" | ||
| if got != want { | ||
| t.Errorf("kustomizeBin() = %v, want %v", got, want) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("fallback to kubectl kustomize when kustomize not found", func(t *testing.T) { | ||
| if _, err := exec.LookPath("kubectl"); err != nil { | ||
| t.Skip("kubectl binary not found in PATH") | ||
| } | ||
| if _, err := exec.LookPath("kustomize"); err == nil { | ||
| t.Skip("kustomize binary found, cannot test fallback") | ||
| } | ||
| r := New() | ||
| got := r.kustomizeBin() | ||
| want := "kubectl kustomize" | ||
| if got != want { | ||
| t.Errorf("kustomizeBin() = %v, want %v", got, want) | ||
| } | ||
| }) |
There was a problem hiding this comment.
These PATH-dependent branches are mostly skipped depending on what binaries happen to be installed (e.g., the fallback case is skipped whenever kustomize exists), so the new fallback logic may not be exercised in CI. Consider making the test deterministic by setting PATH to a temp dir containing a fake kubectl executable (and no kustomize) so exec.LookPath reliably drives the intended branch.
| func TestKubectlKustomizeFallback(t *testing.T) { | ||
| if _, err := exec.LookPath("kubectl"); err != nil { | ||
| t.Skip("kubectl binary not found in PATH") | ||
| } | ||
|
|
||
| t.Run("KustomizeBuild with kubectl kustomize", func(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| srcDir := t.TempDir() | ||
|
|
||
| kustomizationContent := `apiVersion: kustomize.config.k8s.io/v1beta1 | ||
| kind: Kustomization | ||
| resources: | ||
| - deployment.yaml | ||
| ` | ||
| deploymentContent := `apiVersion: apps/v1 | ||
| kind: Deployment | ||
| metadata: | ||
| name: test | ||
| spec: | ||
| replicas: 1 | ||
| selector: | ||
| matchLabels: | ||
| app: test | ||
| template: | ||
| metadata: | ||
| labels: | ||
| app: test | ||
| spec: | ||
| containers: | ||
| - name: test | ||
| image: test:latest | ||
| ` | ||
|
|
||
| templatesDir := filepath.Join(tmpDir, "templates") | ||
| require.NoError(t, os.MkdirAll(templatesDir, 0755)) | ||
|
|
||
| require.NoError(t, os.WriteFile(filepath.Join(srcDir, "kustomization.yaml"), []byte(kustomizationContent), 0644)) | ||
| require.NoError(t, os.WriteFile(filepath.Join(srcDir, "deployment.yaml"), []byte(deploymentContent), 0644)) | ||
|
|
||
| r := New(KustomizeBin("kubectl kustomize")) | ||
|
|
||
| outputFile, err := r.KustomizeBuild(srcDir, tmpDir) | ||
| require.NoError(t, err) | ||
| require.FileExists(t, outputFile) | ||
| }) |
There was a problem hiding this comment.
This test is named as a "fallback" test but it forces KustomizeBin("kubectl kustomize") and skips when kubectl isn’t installed, so it doesn’t verify the automatic fallback behavior in kustomizeBin() and may be skipped in CI. Consider making it self-contained by creating a fake kubectl on PATH (and no kustomize) and asserting New().kustomizeBin() selects it, or rename the test to reflect what it actually validates.
Address review comments: - Add KUSTOMIZE_BIN environment variable support to kustomizeBin() for consistency with helmBin() - Make TestKustomizeBin tests deterministic by using fake executables and controlled PATH instead of relying on system binaries - Make TestKubectlKustomizeFallback test self-contained - Add test for edit commands not supported with kubectl kustomize Signed-off-by: yxxhero <aiopsclub@163.com>
|
@copilot fix comments and deeply review then update the code. |
… to accurately reflect test purpose Agent-Logs-Url: https://github.com/helmfile/chartify/sessions/3fa21b29-e929-497e-b448-a711302394ad Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com>
All three reviewer comments have been addressed:
|
| kustomizeArgs := []string{"--output", renderedFile} | ||
|
|
||
| if !r.isUsingKubectlKustomize() { | ||
| kustomizeArgs = append([]string{"build", tempDir}, kustomizeArgs...) |
There was a problem hiding this comment.
When using the kubectl-based kustomize path, this command invocation never specifies the target directory and also uses r.run (dir=""). That means it will run kubectl kustomize against the process working directory rather than tempDir, producing wrong output or failing. Pass tempDir as the kustomize target (or run in tempDir) for the kubectl case as well, similar to how the non-kubectl path includes build tempDir.
| kustomizeArgs = append([]string{"build", tempDir}, kustomizeArgs...) | |
| kustomizeArgs = append([]string{"build", tempDir}, kustomizeArgs...) | |
| } else { | |
| // When using kubectl kustomize, explicitly pass tempDir as the target directory. | |
| kustomizeArgs = append([]string{tempDir}, kustomizeArgs...) |
| t.Run("KUSTOMIZE_BIN environment variable", func(t *testing.T) { | ||
| if _, ok := os.LookupEnv("KUSTOMIZE_BIN"); ok { | ||
| t.Skip("KUSTOMIZE_BIN environment variable is already set") | ||
| } | ||
| os.Setenv("KUSTOMIZE_BIN", "/custom/kustomize") | ||
| defer os.Unsetenv("KUSTOMIZE_BIN") | ||
| r := New() | ||
| got := r.kustomizeBin() |
There was a problem hiding this comment.
This test mutates KUSTOMIZE_BIN but skips if it’s already set. In environments where KUSTOMIZE_BIN is set (e.g., developer shells), the later subtests that rely on PATH-based lookup will still see the env var and can fail unexpectedly. Prefer saving the original value, temporarily overriding/unsetting it for each subtest that needs PATH lookup, then restoring it in a defer instead of skipping.
| t.Run("KustomizeBuild succeeds with kubectl kustomize option", func(t *testing.T) { | ||
| if _, err := exec.LookPath("kubectl"); err != nil { | ||
| t.Skip("kubectl binary not found in PATH") | ||
| } |
There was a problem hiding this comment.
This test will be skipped on systems without kubectl in PATH. The repo CI workflow installs kustomize but not kubectl (.github/workflows/go.yml), so this coverage is likely skipped in CI and won’t protect the new behavior. Consider stubbing a minimal kubectl script in a temp PATH (similar to TestKustomizeBin) so the test always exercises the kubectl kustomize code path.
| wrappedErr := fmt.Errorf(`%w | ||
|
|
||
| COMMAND: | ||
| %s | ||
| %s | ||
|
|
||
| OUTPUT: | ||
| %s`, | ||
| %s`, |
There was a problem hiding this comment.
The format string adds a leading space before %s while indent(..., " ") already prefixes each line. This results in double-indentation (extra whitespace) in the wrapped error output. Consider removing the extra leading space in the format string (or adjusting indent usage) so the COMMAND/OUTPUT blocks are consistently indented.
|
@copilot fix comments. |
Agent-Logs-Url: https://github.com/helmfile/chartify/sessions/6499602a-94b2-481c-ac49-a2fc2d015090 Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com>
Fixed all 4 reviewer comments in 57fe4e3:
|
| if len(kustomizeOpts.Images) > 0 { | ||
| if r.isUsingKubectlKustomize() { | ||
| return "", fmt.Errorf("setting images via kustomizeOpts.Images is not supported when using 'kubectl kustomize'. Please set images directly in your kustomization.yaml file") | ||
| } |
There was a problem hiding this comment.
The error message leaks the internal struct/field name kustomizeOpts.Images, which isn’t meaningful to callers. Consider rewording to reference the user-facing input (e.g., images configured via values files / KustomizeBuildOpts) and what to do instead when using kubectl kustomize.
| } | ||
| if kustomizeOpts.NamePrefix != "" { | ||
| if r.isUsingKubectlKustomize() { | ||
| return "", fmt.Errorf("setting namePrefix via kustomizeOpts.NamePrefix is not supported when using 'kubectl kustomize'. Please set namePrefix directly in your kustomization.yaml file") |
There was a problem hiding this comment.
This error message references the internal field name kustomizeOpts.NamePrefix. For a better UX, reword to describe the user-facing configuration (values files / build options) and the supported alternative when running via kubectl kustomize.
| return "", fmt.Errorf("setting namePrefix via kustomizeOpts.NamePrefix is not supported when using 'kubectl kustomize'. Please set namePrefix directly in your kustomization.yaml file") | |
| return "", fmt.Errorf("setting namePrefix via Chartify values files or kustomize build options is not supported when using 'kubectl kustomize'. Please set namePrefix directly in your kustomization.yaml file") |
| } | ||
| if kustomizeOpts.NameSuffix != "" { | ||
| if r.isUsingKubectlKustomize() { | ||
| return "", fmt.Errorf("setting nameSuffix via kustomizeOpts.NameSuffix is not supported when using 'kubectl kustomize'. Please set nameSuffix directly in your kustomization.yaml file") |
There was a problem hiding this comment.
This error message references the internal field name kustomizeOpts.NameSuffix. Consider rewording to describe the user-facing setting (values file / build option) and how to achieve the same result when using kubectl kustomize.
| return "", fmt.Errorf("setting nameSuffix via kustomizeOpts.NameSuffix is not supported when using 'kubectl kustomize'. Please set nameSuffix directly in your kustomization.yaml file") | |
| return "", fmt.Errorf("setting nameSuffix via Chartify build options is not supported when using 'kubectl kustomize'. Please set nameSuffix directly in your kustomization.yaml file") |
| } | ||
| if kustomizeOpts.Namespace != "" { | ||
| if r.isUsingKubectlKustomize() { | ||
| return "", fmt.Errorf("setting namespace via kustomizeOpts.Namespace is not supported when using 'kubectl kustomize'. Please set namespace directly in your kustomization.yaml file") |
There was a problem hiding this comment.
This error message references the internal field name kustomizeOpts.Namespace. Consider rewording to reference the user-facing configuration and next steps for kubectl kustomize users (set namespace in kustomization.yaml, etc.).
| return "", fmt.Errorf("setting namespace via kustomizeOpts.Namespace is not supported when using 'kubectl kustomize'. Please set namespace directly in your kustomization.yaml file") | |
| return "", fmt.Errorf("setting namespace via Chartify options is not supported when using 'kubectl kustomize'. Please set namespace in your kustomization.yaml file") |
| // isUsingKubectlKustomize checks if we're using kubectl's built-in kustomize | ||
| func (r *Runner) isUsingKubectlKustomize() bool { | ||
| return r.kustomizeBin() == "kubectl kustomize" | ||
| } |
There was a problem hiding this comment.
isUsingKubectlKustomize() calls r.kustomizeBin(), which may do PATH lookups. It’s invoked multiple times during a single build/patch, so this can repeat exec.LookPath work and could yield inconsistent results if env/PATH changes mid-call. Consider computing bin := r.kustomizeBin() (and the derived bool) once per operation and reusing it, or caching the resolved value on the Runner.
| kustomizeArgs := []string{"--output", renderedFile} | ||
|
|
||
| if !r.isUsingKubectlKustomize() { | ||
| kustomizeArgs = append([]string{"build", tempDir}, kustomizeArgs...) | ||
| } else { | ||
| // kubectl kustomize does not use the "build" subcommand; pass tempDir as the target directly. | ||
| kustomizeArgs = append([]string{tempDir}, kustomizeArgs...) | ||
| } |
There was a problem hiding this comment.
This new branch changes the argument shape for the kustomize invocation when kubectl kustomize is used. There aren’t tests covering the Patch() execution path, so regressions in argument ordering/flags for the kubectl case could slip through. Consider adding a unit test that stubs Runner.RunCommand and asserts the exact binary/args used for both kustomize and kubectl kustomize.
|
|
||
| origPath := os.Getenv("PATH") | ||
| defer os.Setenv("PATH", origPath) | ||
| os.Setenv("PATH", stubDir+":"+origPath) |
There was a problem hiding this comment.
PATH is being updated using a hard-coded ":" separator. Using string(os.PathListSeparator) avoids OS-specific assumptions and keeps the test robust if the CI matrix ever expands beyond Linux.
| os.Setenv("PATH", stubDir+":"+origPath) | |
| os.Setenv("PATH", stubDir+string(os.PathListSeparator)+origPath) |
|
@copilot fix comments |
Agent-Logs-Url: https://github.com/helmfile/chartify/sessions/3bcd32ca-c9c5-4129-9c6c-87e122e320db Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com>
Fixed all 7 reviewer comments in 9eaf7ce:
|
|
@copilot fix ci issue. |
…x writeStubKubectl return Agent-Logs-Url: https://github.com/helmfile/chartify/sessions/d60019c1-5e03-4997-843e-aeb5ca7d0af6 Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com>
Fixed in 4bc0233. The lint job was failing on two issues introduced in the previous commit:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
runner.go:156
runBytessplitscmdusingstrings.Split(cmd, " "), which preserves empty tokens when there are multiple spaces (e.g. "kubectl kustomize" becomes args starting with ""). Now that multi-word commands are supported (e.g. "kubectl kustomize"), this can cause commands configured via env/flags to fail due to incidental whitespace. Consider switching tostrings.Fields(or a shell-quote aware parser) so repeated whitespace doesn’t introduce empty args.
nameArgs := strings.Split(cmd, " ")
name := nameArgs[0]
if len(nameArgs) > 1 {
a := append([]string{}, nameArgs[1:]...)
a = append(a, args...)
args = a
TestKubectlKustomizeFallback→TestKubectlKustomizewith a comment clarifying it tests explicitKustomizeBin("kubectl kustomize")behavior, not the automatic fallback (which is covered inTestKustomizeBin)