From 8197ddecfe4c1b79c5b2c3ea291e1ccf2c2b1369 Mon Sep 17 00:00:00 2001 From: Noah Campbell Date: Thu, 23 Oct 2025 13:20:21 -0500 Subject: [PATCH] added --values and --set flags to lint command (#1907) * added --values and --set flags to lint command * Update lint_test.go --- cmd/preflight/cli/docs.go | 21 +---- cmd/preflight/cli/lint.go | 10 ++- cmd/troubleshoot/cli/lint.go | 10 ++- .../test-error-messages/values-empty.yaml | 2 + .../values-helm-builtins.yaml | 1 + pkg/lint/lint.go | 79 +++++++++++++++++++ pkg/lint/lint_test.go | 73 +++++++++++++---- pkg/lint/types.go | 8 +- pkg/preflight/read_specs.go | 48 ++++++++++- pkg/preflight/read_specs_test.go | 73 +++++++++++++++++ pkg/preflight/template.go | 8 +- pkg/preflight/template_test.go | 2 +- 12 files changed, 284 insertions(+), 51 deletions(-) create mode 100644 examples/test-error-messages/values-empty.yaml create mode 100644 examples/test-error-messages/values-helm-builtins.yaml diff --git a/cmd/preflight/cli/docs.go b/cmd/preflight/cli/docs.go index ddd7a75d..25fd9563 100644 --- a/cmd/preflight/cli/docs.go +++ b/cmd/preflight/cli/docs.go @@ -88,7 +88,7 @@ func extractDocs(templateFiles []string, valuesFiles []string, setValues []strin if err != nil { return errors.Wrapf(err, "failed to load values file %s", valuesFile) } - values = mergeMaps(values, fileValues) + values = preflight.MergeMaps(values, fileValues) } // Normalize maps for Helm set merging @@ -331,25 +331,6 @@ func setNestedValue(m map[string]interface{}, keys []string, value interface{}) } } -func mergeMaps(base, overlay map[string]interface{}) map[string]interface{} { - result := make(map[string]interface{}) - for k, v := range base { - result[k] = v - } - for k, v := range overlay { - if baseVal, exists := result[k]; exists { - if baseMap, ok := baseVal.(map[string]interface{}); ok { - if overlayMap, ok := v.(map[string]interface{}); ok { - result[k] = mergeMaps(baseMap, overlayMap) - continue - } - } - } - result[k] = v - } - return result -} - func renderTemplate(templateContent string, values map[string]interface{}) (string, error) { tmpl := template.New("preflight").Funcs(sprig.FuncMap()) tmpl, err := tmpl.Parse(templateContent) diff --git a/cmd/preflight/cli/lint.go b/cmd/preflight/cli/lint.go index 7ef4811f..a823c9a3 100644 --- a/cmd/preflight/cli/lint.go +++ b/cmd/preflight/cli/lint.go @@ -54,9 +54,11 @@ Exit codes: v := viper.GetViper() opts := lint.LintOptions{ - FilePaths: args, - Fix: v.GetBool("fix"), - Format: v.GetString("format"), + FilePaths: args, + Fix: v.GetBool("fix"), + Format: v.GetString("format"), + ValuesFiles: v.GetStringSlice("values"), + SetValues: v.GetStringSlice("set"), } return runLint(opts) @@ -65,6 +67,8 @@ Exit codes: cmd.Flags().Bool("fix", false, "Automatically fix issues where possible") cmd.Flags().String("format", "text", "Output format: text or json") + cmd.Flags().StringSlice("values", []string{}, "Path to YAML files with template values (required for v1beta3 specs)") + cmd.Flags().StringSlice("set", []string{}, "Set template values via command line (e.g., --set key=value)") return cmd } diff --git a/cmd/troubleshoot/cli/lint.go b/cmd/troubleshoot/cli/lint.go index dd19d701..171e3f69 100644 --- a/cmd/troubleshoot/cli/lint.go +++ b/cmd/troubleshoot/cli/lint.go @@ -54,9 +54,11 @@ Exit codes: v := viper.GetViper() opts := lint.LintOptions{ - FilePaths: args, - Fix: v.GetBool("fix"), - Format: v.GetString("format"), + FilePaths: args, + Fix: v.GetBool("fix"), + Format: v.GetString("format"), + ValuesFiles: v.GetStringSlice("values"), + SetValues: v.GetStringSlice("set"), } return runLint(opts) @@ -65,6 +67,8 @@ Exit codes: cmd.Flags().Bool("fix", false, "Automatically fix issues where possible") cmd.Flags().String("format", "text", "Output format: text or json") + cmd.Flags().StringSlice("values", []string{}, "Path to YAML files with template values (required for v1beta3 specs)") + cmd.Flags().StringSlice("set", []string{}, "Set template values via command line (e.g., --set key=value)") return cmd } diff --git a/examples/test-error-messages/values-empty.yaml b/examples/test-error-messages/values-empty.yaml new file mode 100644 index 00000000..4a828fd2 --- /dev/null +++ b/examples/test-error-messages/values-empty.yaml @@ -0,0 +1,2 @@ +# Empty values file for v1beta3 specs without templates +{} diff --git a/examples/test-error-messages/values-helm-builtins.yaml b/examples/test-error-messages/values-helm-builtins.yaml new file mode 100644 index 00000000..0bfcb3d3 --- /dev/null +++ b/examples/test-error-messages/values-helm-builtins.yaml @@ -0,0 +1 @@ +minVersion: "1.19.0" diff --git a/pkg/lint/lint.go b/pkg/lint/lint.go index 8344bdec..6b23fe41 100644 --- a/pkg/lint/lint.go +++ b/pkg/lint/lint.go @@ -9,6 +9,8 @@ import ( "github.com/pkg/errors" "github.com/replicatedhq/troubleshoot/internal/util" "github.com/replicatedhq/troubleshoot/pkg/constants" + "github.com/replicatedhq/troubleshoot/pkg/preflight" + "helm.sh/helm/v3/pkg/strvals" "sigs.k8s.io/yaml" ) @@ -27,6 +29,72 @@ func LintFiles(opts LintOptions) ([]LintResult, error) { } fileContent := string(fileBytes) + // Check if this is a v1beta3 spec + isV1Beta3 := detectAPIVersionFromContent(fileContent) == constants.Troubleshootv1beta3Kind + isV1Beta2 := detectAPIVersionFromContent(fileContent) == constants.Troubleshootv1beta2Kind + + // Check if the content has Helm templates (for preflight v1beta3) + hasTemplates := strings.Contains(fileContent, "{{") && strings.Contains(fileContent, "}}") + + // Track if we should add a warning about unused values for v1beta2 + hasUnusedValuesWarning := isV1Beta2 && (len(opts.ValuesFiles) > 0 || len(opts.SetValues) > 0) + + // If v1beta3 with templates, require values and render the template + if isV1Beta3 && hasTemplates { + if len(opts.ValuesFiles) == 0 && len(opts.SetValues) == 0 { + return nil, errors.New("v1beta3 specs with Helm templates require a values file. Please provide values using --values or --set flags") + } + + // Load values from files and --set flags + values := make(map[string]interface{}) + for _, valuesFile := range opts.ValuesFiles { + if valuesFile == "" { + continue + } + data, err := os.ReadFile(valuesFile) + if err != nil { + return nil, errors.Wrapf(err, "failed to read values file %s", valuesFile) + } + + var fileValues map[string]interface{} + if err := yaml.Unmarshal(data, &fileValues); err != nil { + return nil, errors.Wrapf(err, "failed to parse values file %s", valuesFile) + } + + values = preflight.MergeMaps(values, fileValues) + } + + // Apply --set values + for _, setValue := range opts.SetValues { + if err := strvals.ParseInto(setValue, values); err != nil { + return nil, errors.Wrapf(err, "failed to parse --set value: %s", setValue) + } + } + + // Render the template + preflight.SeedDefaultBooleans(fileContent, values) + preflight.SeedParentMapsForValueRefs(fileContent, values) + rendered, err := preflight.RenderWithHelmTemplate(fileContent, values) + if err != nil { + // If rendering fails, create a result with the render error + // This allows us to report template syntax errors + results = append(results, LintResult{ + FilePath: filePath, + Errors: []LintError{ + { + Line: 1, + Message: fmt.Sprintf("Failed to render v1beta3 template: %v", err), + Field: "template", + }, + }, + }) + continue + } + + // Use the rendered content for linting + fileContent = rendered + } + // Split into YAML documents docs := util.SplitYAML(fileContent) @@ -101,6 +169,17 @@ func LintFiles(opts LintOptions) ([]LintResult, error) { } } + // Add warning if values were provided for a v1beta2 spec + if hasUnusedValuesWarning { + fileResult.Warnings = append([]LintWarning{ + { + Line: 1, + Message: "Values files provided but this is a v1beta2 spec. Values are only used with v1beta3 specs. Did you mean to use apiVersion: troubleshoot.sh/v1beta3?", + Field: "apiVersion", + }, + }, fileResult.Warnings...) + } + if writeNeeded { // Reassemble with the same delimiter used by util.SplitYAML updated := strings.Join(newDocs, "\n---\n") diff --git a/pkg/lint/lint_test.go b/pkg/lint/lint_test.go index 8dfa50bc..f8b141ea 100644 --- a/pkg/lint/lint_test.go +++ b/pkg/lint/lint_test.go @@ -15,6 +15,7 @@ func TestLintMultipleFiles(t *testing.T) { tests := []struct { name string files []string + valuesFiles []string // values files for v1beta3 specs expectErrors map[string][]string // filename -> expected error substrings expectWarnings map[string][]string // filename -> expected warning substrings expectPass map[string]bool // filename -> should pass without errors @@ -24,14 +25,13 @@ func TestLintMultipleFiles(t *testing.T) { files: []string{ "helm-builtins-v1beta3.yaml", }, - expectErrors: map[string][]string{}, - expectWarnings: map[string][]string{ - "helm-builtins-v1beta3.yaml": { - "Template values that must be provided at runtime: minVersion", - }, + valuesFiles: []string{ + "values-helm-builtins.yaml", }, + expectErrors: map[string][]string{}, + expectWarnings: map[string][]string{}, expectPass: map[string]bool{ - "helm-builtins-v1beta3.yaml": false, // has warnings + "helm-builtins-v1beta3.yaml": true, }, }, { @@ -39,6 +39,9 @@ func TestLintMultipleFiles(t *testing.T) { files: []string{ "invalid-collectors-analyzers.yaml", }, + valuesFiles: []string{ + "values-empty.yaml", + }, expectErrors: map[string][]string{ "invalid-collectors-analyzers.yaml": { // The linter may stop early due to structural issues @@ -57,6 +60,9 @@ func TestLintMultipleFiles(t *testing.T) { "missing-metadata-v1beta3.yaml", "no-analyzers-v1beta3.yaml", }, + valuesFiles: []string{ + "values-empty.yaml", + }, expectErrors: map[string][]string{ "missing-apiversion-v1beta3.yaml": { "Missing or empty 'apiVersion' field", @@ -95,6 +101,9 @@ func TestLintMultipleFiles(t *testing.T) { "support-bundle-no-collectors-v1beta3.yaml", "support-bundle-valid-v1beta3.yaml", }, + valuesFiles: []string{ + "values-empty.yaml", + }, expectErrors: map[string][]string{ "support-bundle-no-collectors-v1beta3.yaml": { "SupportBundle spec must contain 'collectors' or 'hostCollectors'", @@ -112,6 +121,9 @@ func TestLintMultipleFiles(t *testing.T) { "missing-metadata-v1beta3.yaml", "wrong-apiversion-v1beta3.yaml", }, + valuesFiles: []string{ + "values-empty.yaml", + }, expectErrors: map[string][]string{ "missing-metadata-v1beta3.yaml": { "Missing 'metadata' section", @@ -142,11 +154,18 @@ func TestLintMultipleFiles(t *testing.T) { } } + // Build values file paths + var valuesFilePaths []string + for _, vf := range tt.valuesFiles { + valuesFilePaths = append(valuesFilePaths, filepath.Join(testDir, vf)) + } + // Run linter opts := LintOptions{ - FilePaths: filePaths, - Fix: false, - Format: "text", + FilePaths: filePaths, + Fix: false, + Format: "text", + ValuesFiles: valuesFilePaths, } results, err := LintFiles(opts) @@ -246,7 +265,7 @@ spec: fixedContent: "apiVersion: troubleshoot.sh/v1beta3", }, { - name: "fix missing leading dot in template", + name: "v1beta3 template syntax error is reported", content: `apiVersion: troubleshoot.sh/v1beta3 kind: Preflight metadata: @@ -258,11 +277,17 @@ spec: - pass: when: '>= 1.19.0' message: OK`, - expectFix: true, - fixedContent: "{{ .Values.name }}", + expectFix: false, // Template errors in v1beta3 are not auto-fixable, rendering will fail + fixedContent: "Failed to render v1beta3 template", // Expect an error message }, } + // Create empty values file for v1beta3 tests + emptyValuesFile := filepath.Join(tmpDir, "values-empty.yaml") + if err := os.WriteFile(emptyValuesFile, []byte("{}"), 0644); err != nil { + t.Fatalf("Failed to write empty values file: %v", err) + } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Write test content to temp file @@ -273,9 +298,10 @@ spec: // Run linter with fix enabled opts := LintOptions{ - FilePaths: []string{testFile}, - Fix: true, - Format: "text", + FilePaths: []string{testFile}, + Fix: true, + Format: "text", + ValuesFiles: []string{emptyValuesFile}, } results, err := LintFiles(opts) @@ -294,12 +320,27 @@ spec: } fixedContent := string(fixedBytes) - // Check if fix was applied + // Check if fix was applied or error was reported if tt.expectFix { if !strings.Contains(fixedContent, tt.fixedContent) { t.Errorf("Expected fixed content to contain '%s', but got:\n%s", tt.fixedContent, fixedContent) } + } else { + // For tests that don't expect fix, check for errors + if len(results[0].Errors) > 0 { + errorFound := false + for _, err := range results[0].Errors { + if strings.Contains(err.Message, tt.fixedContent) { + errorFound = true + break + } + } + if !errorFound { + t.Errorf("Expected error containing '%s', but got errors: %v", + tt.fixedContent, results[0].Errors) + } + } } }) } diff --git a/pkg/lint/types.go b/pkg/lint/types.go index a95b9ff8..62178e74 100644 --- a/pkg/lint/types.go +++ b/pkg/lint/types.go @@ -23,9 +23,11 @@ type LintWarning struct { } type LintOptions struct { - FilePaths []string - Fix bool - Format string // "text" or "json" + FilePaths []string + Fix bool + Format string // "text" or "json" + ValuesFiles []string // Path to YAML files with template values (for v1beta3) + SetValues []string // Template values from command line (for v1beta3) } // HasErrors returns true if any of the results contain errors diff --git a/pkg/preflight/read_specs.go b/pkg/preflight/read_specs.go index 0167e048..1514ac7e 100644 --- a/pkg/preflight/read_specs.go +++ b/pkg/preflight/read_specs.go @@ -88,6 +88,52 @@ func preprocessV1Beta3Specs(args []string) ([]string, []string, error) { valuesFiles := viper.GetStringSlice("values") setValues := viper.GetStringSlice("set") + // Check if any args contain v1beta3 specs + hasV1Beta3 := false + for _, arg := range args { + // Skip non-file arguments + if arg == "-" || strings.HasPrefix(arg, "http://") || strings.HasPrefix(arg, "https://") || + strings.HasPrefix(arg, "secret/") || strings.HasPrefix(arg, "configmap/") { + continue + } + + // Check if file exists + if _, err := os.Stat(arg); err != nil { + continue + } + + // Read the file + content, err := os.ReadFile(arg) + if err != nil { + continue + } + + // Check if it's a v1beta3 spec with templates + contentStr := string(content) + var parsed map[string]interface{} + if err := yaml.Unmarshal(content, &parsed); err == nil { + if apiVersion, ok := parsed["apiVersion"]; ok && apiVersion == constants.Troubleshootv1beta3Kind { + // Only require values if the spec has Helm templates + if strings.Contains(contentStr, "{{") && strings.Contains(contentStr, "}}") { + hasV1Beta3 = true + break + } + } + } else { + // If YAML parsing fails, check raw content for v1beta3 with templates + if strings.Contains(contentStr, "apiVersion: troubleshoot.sh/v1beta3") && + strings.Contains(contentStr, "{{") && strings.Contains(contentStr, "}}") { + hasV1Beta3 = true + break + } + } + } + + // If v1beta3 spec with templates found but no values provided, return error + if hasV1Beta3 && len(valuesFiles) == 0 && len(setValues) == 0 { + return nil, nil, errors.New("v1beta3 specs with Helm templates require a values file. Please provide values using --values or --set flags") + } + // If no values provided, return args unchanged if len(valuesFiles) == 0 && len(setValues) == 0 { return args, nil, nil @@ -109,7 +155,7 @@ func preprocessV1Beta3Specs(args []string) ([]string, []string, error) { return nil, nil, errors.Wrapf(err, "failed to parse values file %s", valuesFile) } - values = mergeMaps(values, fileValues) + values = MergeMaps(values, fileValues) } // Apply --set values diff --git a/pkg/preflight/read_specs_test.go b/pkg/preflight/read_specs_test.go index 69b03d59..31407a99 100644 --- a/pkg/preflight/read_specs_test.go +++ b/pkg/preflight/read_specs_test.go @@ -8,6 +8,7 @@ import ( "github.com/replicatedhq/troubleshoot/internal/testutils" troubleshootv1beta2 "github.com/replicatedhq/troubleshoot/pkg/apis/troubleshoot/v1beta2" "github.com/replicatedhq/troubleshoot/pkg/loader" + "github.com/spf13/viper" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -410,3 +411,75 @@ func singleTestPreflightSpecsRead(t *testing.T, tt *PreflightSpecsReadTest) (*lo return kinds, err } + +func TestPreprocessV1Beta3Specs_RequiresValues(t *testing.T) { + // Save and restore viper state + oldValues := viper.Get("values") + oldSet := viper.Get("set") + defer func() { + viper.Set("values", oldValues) + viper.Set("set", oldSet) + }() + + t.Run("v1beta3 without values should error", func(t *testing.T) { + // Clear viper values + viper.Set("values", []string{}) + viper.Set("set", []string{}) + + v1beta3File := filepath.Join(testutils.FileDir(), "../../examples/preflight/simple-v1beta3.yaml") + _, _, err := preprocessV1Beta3Specs([]string{v1beta3File}) + + require.Error(t, err) + assert.Contains(t, err.Error(), "v1beta3 specs with Helm templates require a values file") + }) + + t.Run("v1beta3 with values file should succeed", func(t *testing.T) { + valuesFile := filepath.Join(testutils.FileDir(), "../../examples/preflight/values-v1beta3-1.yaml") + viper.Set("values", []string{valuesFile}) + viper.Set("set", []string{}) + + v1beta3File := filepath.Join(testutils.FileDir(), "../../examples/preflight/simple-v1beta3.yaml") + processedArgs, tempFiles, err := preprocessV1Beta3Specs([]string{v1beta3File}) + + // Clean up temp files + defer func() { + for _, f := range tempFiles { + _ = os.Remove(f) + } + }() + + require.NoError(t, err) + assert.NotNil(t, processedArgs) + }) + + t.Run("v1beta3 with --set values should succeed", func(t *testing.T) { + viper.Set("values", []string{}) + viper.Set("set", []string{"kubernetes.enabled=true"}) + + v1beta3File := filepath.Join(testutils.FileDir(), "../../examples/preflight/simple-v1beta3.yaml") + processedArgs, tempFiles, err := preprocessV1Beta3Specs([]string{v1beta3File}) + + // Clean up temp files + defer func() { + for _, f := range tempFiles { + _ = os.Remove(f) + } + }() + + require.NoError(t, err) + assert.NotNil(t, processedArgs) + }) + + t.Run("v1beta2 without values should succeed", func(t *testing.T) { + viper.Set("values", []string{}) + viper.Set("set", []string{}) + + v1beta2File := filepath.Join(testutils.FileDir(), "../../testdata/preflightspec/troubleshoot_v1beta2_preflight_gotest.yaml") + processedArgs, tempFiles, err := preprocessV1Beta3Specs([]string{v1beta2File}) + + require.NoError(t, err) + assert.NotNil(t, processedArgs) + assert.Empty(t, tempFiles) + assert.Equal(t, []string{v1beta2File}, processedArgs) + }) +} diff --git a/pkg/preflight/template.go b/pkg/preflight/template.go index 95579d80..c3f6c555 100644 --- a/pkg/preflight/template.go +++ b/pkg/preflight/template.go @@ -33,7 +33,7 @@ func RunTemplate(templateFile string, valuesFiles []string, setValues []string, if err != nil { return errors.Wrapf(err, "failed to load values file %s", valuesFile) } - values = mergeMaps(values, fileValues) + values = MergeMaps(values, fileValues) } // Apply --set values (Helm semantics) @@ -165,8 +165,8 @@ func cleanRenderedYAML(content string) string { return strings.Join(cleaned, "\n") + "\n" } -// mergeMaps recursively merges two maps -func mergeMaps(base, overlay map[string]interface{}) map[string]interface{} { +// MergeMaps recursively merges two maps, with overlay taking precedence +func MergeMaps(base, overlay map[string]interface{}) map[string]interface{} { result := make(map[string]interface{}) // Copy base map @@ -180,7 +180,7 @@ func mergeMaps(base, overlay map[string]interface{}) map[string]interface{} { // If both are maps, merge recursively if baseMap, ok := baseVal.(map[string]interface{}); ok { if overlayMap, ok := v.(map[string]interface{}); ok { - result[k] = mergeMaps(baseMap, overlayMap) + result[k] = MergeMaps(baseMap, overlayMap) continue } } diff --git a/pkg/preflight/template_test.go b/pkg/preflight/template_test.go index b2a689ff..89b5c35d 100644 --- a/pkg/preflight/template_test.go +++ b/pkg/preflight/template_test.go @@ -430,7 +430,7 @@ func TestRender_V1Beta3_MergeMultipleValuesFiles_And_SetPrecedence(t *testing.T) for _, f := range []string{minimalFile, file1, file3} { m, err := loadValuesFile(f) require.NoError(t, err) - vals = mergeMaps(vals, m) + vals = MergeMaps(vals, m) } // First render without --set; expect NO kubernetes analyzer