mirror of
https://github.com/helmfile/helmfile.git
synced 2026-10-18 01:31:36 +02:00
fix: resolve --validate flag conflict with kustomize in Helm 4 (#2362)
* fix: resolve --validate flag conflict with kustomize in Helm 4 Fixes #2355 In Helm 4, the --validate and --dry-run flags are mutually exclusive. When using kustomize/chartify charts with helmfile diff --validate, the code was adding both --validate AND --dry-run=server to the helm template command, causing the error: Error: if any flags in the group [validate dry-run] are set none of the others can be; [dry-run validate] were all set The fix checks if --validate is already set before adding --dry-run=server. Since --validate already provides server-side validation (it was deprecated in favor of --dry-run=server in Helm 4), adding --dry-run=server is redundant when --validate is present. Changes: - Add !opts.Validate condition to processChartification() in state.go - Add comprehensive unit tests for validate/dry-run mutual exclusion - Add integration test with kustomize chart to prevent regression Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * address review feedback from Copilot - Add missing test cases for destroy, delete, test, status WITH --validate - Update integration test to use 'diff' instead of 'template' to properly exercise the cluster-requiring code path that triggers --dry-run=server - Add sync warning comments to the test helper function noting it must be kept in sync with processChartification() in state.go Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * add missing 'build with validate' test case Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * address additional review feedback from Copilot - Fix integration test to capture output and exit code in single execution instead of running helmfile twice (more efficient) - Add detailed documentation explaining why test helper duplication is intentional: extracting shared function would require exposing internal API and complex refactoring of processChartification dependencies - Note that integration test exercises actual code path end-to-end Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * fix: correct go doc comment formatting for gci linter Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * fix: update line number reference from 1497-1523 to 1497-1524 Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> --------- Signed-off-by: Aditya Menon <amenon@canarytechnologies.com>
This commit is contained in:
Aditya Menon
@@ -0,0 +1,176 @@
|
||||
package state
|
||||
|
||||
import (
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// TestValidateAndDryRunMutualExclusion verifies that when --validate is set,
|
||||
// --dry-run=server is NOT added to TemplateArgs in Helm 4 compatibility.
|
||||
// This is a regression test for issue #2355.
|
||||
//
|
||||
// Background: In Helm 4, the --validate and --dry-run flags are mutually exclusive.
|
||||
// When helmfile uses kustomize/chartify, it was adding --dry-run=server for cluster-
|
||||
// requiring commands (like diff, apply) to support the lookup() function. However,
|
||||
// if --validate is already set, we should NOT add --dry-run=server because:
|
||||
// 1. They are mutually exclusive in Helm 4
|
||||
// 2. --validate already provides server-side validation
|
||||
func TestValidateAndDryRunMutualExclusion(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
helmfileCommand string
|
||||
validate bool
|
||||
expectedDryRun bool // Should --dry-run=server be added?
|
||||
}{
|
||||
// Cluster-requiring commands without --validate should get --dry-run=server
|
||||
{"diff without validate", "diff", false, true},
|
||||
{"apply without validate", "apply", false, true},
|
||||
{"sync without validate", "sync", false, true},
|
||||
{"destroy without validate", "destroy", false, true},
|
||||
{"delete without validate", "delete", false, true},
|
||||
{"test without validate", "test", false, true},
|
||||
{"status without validate", "status", false, true},
|
||||
|
||||
// Cluster-requiring commands WITH --validate should NOT get --dry-run=server
|
||||
// This is the fix for issue #2355
|
||||
{"diff with validate", "diff", true, false},
|
||||
{"apply with validate", "apply", true, false},
|
||||
{"sync with validate", "sync", true, false},
|
||||
{"destroy with validate", "destroy", true, false},
|
||||
{"delete with validate", "delete", true, false},
|
||||
{"test with validate", "test", true, false},
|
||||
{"status with validate", "status", true, false},
|
||||
|
||||
// Non-cluster commands should never get --dry-run=server
|
||||
{"template without validate", "template", false, false},
|
||||
{"template with validate", "template", true, false},
|
||||
{"lint without validate", "lint", false, false},
|
||||
{"lint with validate", "lint", true, false},
|
||||
{"build without validate", "build", false, false},
|
||||
{"build with validate", "build", true, false},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
// Simulate the logic from processChartification
|
||||
templateArgs := shouldAddDryRunServer(tt.helmfileCommand, tt.validate, "")
|
||||
|
||||
hasDryRun := strings.Contains(templateArgs, "--dry-run=server")
|
||||
|
||||
if hasDryRun != tt.expectedDryRun {
|
||||
t.Errorf("shouldAddDryRunServer(%q, validate=%v) = %q, hasDryRun=%v; want hasDryRun=%v",
|
||||
tt.helmfileCommand, tt.validate, templateArgs, hasDryRun, tt.expectedDryRun)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestDryRunServerWithExistingTemplateArgs verifies that --dry-run=server is
|
||||
// appended correctly when there are existing template args.
|
||||
func TestDryRunServerWithExistingTemplateArgs(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
helmfileCommand string
|
||||
validate bool
|
||||
existingArgs string
|
||||
expectedContains string
|
||||
shouldHaveDryRun bool
|
||||
}{
|
||||
{
|
||||
name: "append to existing args when validate is false",
|
||||
helmfileCommand: "diff",
|
||||
validate: false,
|
||||
existingArgs: "--some-flag",
|
||||
expectedContains: "--dry-run=server",
|
||||
shouldHaveDryRun: true,
|
||||
},
|
||||
{
|
||||
name: "do not append when validate is true",
|
||||
helmfileCommand: "diff",
|
||||
validate: true,
|
||||
existingArgs: "--some-flag",
|
||||
expectedContains: "--some-flag",
|
||||
shouldHaveDryRun: false,
|
||||
},
|
||||
{
|
||||
name: "do not duplicate if dry-run already exists",
|
||||
helmfileCommand: "diff",
|
||||
validate: false,
|
||||
existingArgs: "--dry-run=client",
|
||||
expectedContains: "--dry-run=client",
|
||||
shouldHaveDryRun: false, // Already has --dry-run, should not add server
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
templateArgs := shouldAddDryRunServer(tt.helmfileCommand, tt.validate, tt.existingArgs)
|
||||
|
||||
if !strings.Contains(templateArgs, tt.expectedContains) {
|
||||
t.Errorf("shouldAddDryRunServer() = %q; want to contain %q",
|
||||
templateArgs, tt.expectedContains)
|
||||
}
|
||||
|
||||
hasDryRunServer := strings.Contains(templateArgs, "--dry-run=server")
|
||||
if hasDryRunServer != tt.shouldHaveDryRun {
|
||||
t.Errorf("shouldAddDryRunServer() = %q; hasDryRunServer=%v, want %v",
|
||||
templateArgs, hasDryRunServer, tt.shouldHaveDryRun)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// shouldAddDryRunServer determines whether to add --dry-run=server to template args.
|
||||
// This helper function encapsulates the logic from processChartification for testing.
|
||||
//
|
||||
// NOTE ON DUPLICATION: This function intentionally duplicates the command classification
|
||||
// logic from processChartification() in state.go (lines 1497-1524). While extracting this
|
||||
// into a shared function would reduce duplication, it would require:
|
||||
// 1. Exposing internal implementation details in the public API
|
||||
// 2. Complex refactoring of processChartification which has many dependencies (chartify
|
||||
// library, filesystem, HelmState)
|
||||
//
|
||||
// For this focused bug fix, the duplication is acceptable because:
|
||||
// - The integration test (test/integration/test-cases/issue-2355.sh) exercises the actual
|
||||
// processChartification code path end-to-end
|
||||
// - This unit test documents the expected behavior and catches regressions quickly
|
||||
// - The logic being tested is simple and unlikely to change frequently
|
||||
//
|
||||
// SYNC WARNING: If the command classification in processChartification() changes
|
||||
// (state.go lines 1497-1507), this function must be updated to match.
|
||||
//
|
||||
// Parameters:
|
||||
// - helmfileCommand: the helmfile command being run (diff, apply, template, etc.)
|
||||
// - validate: whether the --validate flag was passed
|
||||
// - existingTemplateArgs: any existing template arguments
|
||||
//
|
||||
// Returns the updated template args string.
|
||||
func shouldAddDryRunServer(helmfileCommand string, validate bool, existingTemplateArgs string) string {
|
||||
// Determine if the command requires cluster access
|
||||
// SYNC: Keep in sync with processChartification() in state.go lines 1497-1507
|
||||
var requiresCluster bool
|
||||
switch helmfileCommand {
|
||||
case "diff", "apply", "sync", "destroy", "delete", "test", "status":
|
||||
requiresCluster = true
|
||||
case "template", "lint", "build", "pull", "fetch", "write-values", "list", "show-dag", "deps", "repos", "cache", "init", "completion", "help", "version":
|
||||
requiresCluster = false
|
||||
default:
|
||||
requiresCluster = true
|
||||
}
|
||||
|
||||
templateArgs := existingTemplateArgs
|
||||
|
||||
// Issue #2355: In Helm 4, --validate and --dry-run are mutually exclusive.
|
||||
// Only add --dry-run=server if:
|
||||
// 1. The command requires cluster access
|
||||
// 2. --validate is NOT set (to avoid mutual exclusion error)
|
||||
if requiresCluster && !validate {
|
||||
if templateArgs == "" {
|
||||
templateArgs = "--dry-run=server"
|
||||
} else if !strings.Contains(templateArgs, "--dry-run") {
|
||||
templateArgs += " --dry-run=server"
|
||||
}
|
||||
}
|
||||
|
||||
return templateArgs
|
||||
}
|
||||
@@ -120,6 +120,7 @@ ${kubectl} create namespace ${test_ns} || fail "Could not create namespace ${tes
|
||||
. ${dir}/test-cases/oci-parallel-pull.sh
|
||||
. ${dir}/test-cases/issue-2297-local-chart-transformers.sh
|
||||
. ${dir}/test-cases/issue-2309-kube-context-template.sh
|
||||
. ${dir}/test-cases/issue-2355.sh
|
||||
|
||||
# ALL DONE -----------------------------------------------------------------------------------------------------------
|
||||
|
||||
|
||||
Executable
+42
@@ -0,0 +1,42 @@
|
||||
#!/usr/bin/env bash
|
||||
|
||||
# Regression test for issue #2355: Validate flag does not work when using kustomize after Helm 4 upgrade
|
||||
#
|
||||
# Background: In Helm 4, the --validate and --dry-run flags are mutually exclusive.
|
||||
# When helmfile uses kustomize/chartify, it was incorrectly adding --dry-run=server
|
||||
# even when --validate was already set, causing:
|
||||
# Error: if any flags in the group [validate dry-run] are set none of the others can be
|
||||
|
||||
issue_2355_input_dir="${cases_dir}/issue-2355/input"
|
||||
|
||||
test_start "issue-2355: validate flag with kustomize charts (Helm 4 compatibility)"
|
||||
|
||||
# Test 1: helmfile diff --validate with kustomize chart should NOT fail due to validate/dry-run mutual exclusion
|
||||
# We deliberately use diff here because it is a cluster-requiring command that can trigger --dry-run=server via chartify.
|
||||
info "Test 1: Running diff --validate with kustomize chart"
|
||||
error_output=$(${helmfile} -f ${issue_2355_input_dir}/helmfile.yaml diff --validate 2>&1)
|
||||
exit_code=$?
|
||||
if [ $exit_code -ne 0 ]; then
|
||||
# Check if it's the specific mutual exclusion error we're fixing
|
||||
if echo "$error_output" | grep -q "validate.*dry-run.*were all set"; then
|
||||
fail "helmfile diff --validate with kustomize failed with mutual exclusion error (issue #2355 not fixed): $error_output"
|
||||
else
|
||||
# Other errors might be acceptable (e.g., no cluster connection for validation, or non-zero diff exit codes)
|
||||
warn "helmfile diff --validate had an error (but not the mutual exclusion issue): $error_output"
|
||||
fi
|
||||
fi
|
||||
|
||||
# Test 2: Verify that without --validate, the command does not hit the mutual exclusion error
|
||||
info "Test 2: Running diff without --validate (baseline test for flag interaction)"
|
||||
error_output=$(${helmfile} -f ${issue_2355_input_dir}/helmfile.yaml diff 2>&1)
|
||||
exit_code=$?
|
||||
if [ $exit_code -ne 0 ]; then
|
||||
if echo "$error_output" | grep -q "validate.*dry-run.*were all set"; then
|
||||
fail "helmfile diff without --validate failed with mutual exclusion error (issue #2355 not fixed): $error_output"
|
||||
else
|
||||
# Non-zero exit codes from diff (e.g., detected differences) or other errors are tolerated here.
|
||||
warn "helmfile diff without --validate had an error (but not the mutual exclusion issue): $error_output"
|
||||
fi
|
||||
fi
|
||||
|
||||
test_pass "issue-2355: validate flag with kustomize charts (Helm 4 compatibility)"
|
||||
@@ -0,0 +1,7 @@
|
||||
# Test helmfile for issue #2355
|
||||
# This helmfile uses a local kustomize chart to test the --validate flag compatibility with Helm 4
|
||||
|
||||
releases:
|
||||
- name: test-kustomize-2355
|
||||
chart: ./kubernetes
|
||||
namespace: helmfile-tests
|
||||
@@ -0,0 +1,21 @@
|
||||
apiVersion: apps/v1
|
||||
kind: Deployment
|
||||
metadata:
|
||||
name: test-nginx-2355
|
||||
labels:
|
||||
app: test-2355
|
||||
spec:
|
||||
replicas: 1
|
||||
selector:
|
||||
matchLabels:
|
||||
app: test-2355
|
||||
template:
|
||||
metadata:
|
||||
labels:
|
||||
app: test-2355
|
||||
spec:
|
||||
containers:
|
||||
- name: nginx
|
||||
image: nginx:latest
|
||||
ports:
|
||||
- containerPort: 80
|
||||
@@ -0,0 +1,4 @@
|
||||
apiVersion: kustomize.config.k8s.io/v1beta1
|
||||
kind: Kustomization
|
||||
resources:
|
||||
- deployment.yaml
|
||||
Reference in New Issue
Block a user