From f06bdf0d75dafb94c4f74fbdd0697fc9a2ff9c22 Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Wed, 4 Feb 2026 19:31:27 +0000 Subject: [PATCH 1/2] Fix YAML replacement affecting wrong fields with same value The surgicalReplace function was searching the entire file for pattern matches, causing issues when multiple fields have the same version value but different formatting (e.g., `version: 0.9.0` and `appVersion: "0.9.0"`). The double-quoted pattern would match the quoted appVersion first, even when updating the unquoted version field, corrupting the wrong field. Changes: - Add extractKeyFromPath() to get the YAML key name from a path - Make replacement patterns key-aware to only match the specific field - Fix Go regex capture group syntax (use ${1} instead of $1) to avoid ambiguity when new version starts with a digit - Add regression tests for the fix Fixes an issue where updating version in Chart.yaml would accidentally modify appVersion, causing version mismatch errors on subsequent runs. Co-Authored-By: Claude Opus 4.5 --- internal/files/yaml.go | 90 +++++++++++++++-------- internal/files/yaml_test.go | 140 ++++++++++++++++++++++++++++++++++++ 2 files changed, 200 insertions(+), 30 deletions(-) diff --git a/internal/files/yaml.go b/internal/files/yaml.go index 33283cb..6daa634 100644 --- a/internal/files/yaml.go +++ b/internal/files/yaml.go @@ -82,8 +82,11 @@ func UpdateYAMLFile(cfg VersionFileConfig, currentVersion, newVersion string) er newValue = newVersionStr } - // Perform surgical replacement - find and replace only the value - newData, err := surgicalReplace(data, oldValue, newValue) + // Extract the key name from the path for targeted replacement + key := extractKeyFromPath(cfg.Path) + + // Perform surgical replacement - find and replace only the value for this specific key + newData, err := surgicalReplace(data, key, oldValue, newValue) if err != nil { return fmt.Errorf("replacing value at path %s: %w", cfg.Path, err) } @@ -96,84 +99,111 @@ func UpdateYAMLFile(cfg VersionFileConfig, currentVersion, newVersion string) er return nil } +// extractKeyFromPath extracts the final key name from a dot-notation path. +// Examples: +// +// "version" -> "version" +// "metadata.version" -> "version" +// "spec.containers[0].image" -> "image" +// "operator.image" -> "image" +func extractKeyFromPath(path string) string { + // Remove any array indices from the path + re := regexp.MustCompile(`\[\d+\]`) + cleanPath := re.ReplaceAllString(path, "") + + // Get the last component after splitting by dots + parts := strings.Split(cleanPath, ".") + if len(parts) == 0 { + return path + } + return parts[len(parts)-1] +} + // replacementRule defines a pattern-replacement pair for surgical YAML value replacement. // Each rule targets a specific quote style or value format in YAML files. type replacementRule struct { // name describes what this rule handles (for debugging/documentation) name string - // pattern returns the regex pattern to match for the given old value - pattern func(oldValue string) string - // replacement returns the replacement string for the given new value - replacement func(newValue string) string + // pattern returns the regex pattern to match for the given key and old value. + // The key is included to ensure we only match the specific YAML key we're updating. + pattern func(key, oldValue string) string + // replacement returns the replacement string for the given key and new value + replacement func(key, newValue string) string } // replacementRules defines all the rules for surgical YAML value replacement. // Rules are tried in order; the first matching rule is applied. +// Each pattern includes the key name to ensure we only replace the value for the +// specific YAML key being updated, not other keys with the same value. var replacementRules = []replacementRule{ { // Handles double-quoted values: key: "value" name: "double-quoted", - pattern: func(oldValue string) string { - return fmt.Sprintf(`"(%s)"`, regexp.QuoteMeta(oldValue)) + pattern: func(key, oldValue string) string { + return fmt.Sprintf(`(%s:\s*)"(%s)"`, regexp.QuoteMeta(key), regexp.QuoteMeta(oldValue)) }, - replacement: func(newValue string) string { - return fmt.Sprintf(`"%s"`, newValue) + replacement: func(key, newValue string) string { + // Use ${1} syntax to avoid ambiguity when newValue starts with a digit + return fmt.Sprintf(`${1}"%s"`, newValue) }, }, { // Handles single-quoted values: key: 'value' name: "single-quoted", - pattern: func(oldValue string) string { - return fmt.Sprintf(`'(%s)'`, regexp.QuoteMeta(oldValue)) + pattern: func(key, oldValue string) string { + return fmt.Sprintf(`(%s:\s*)'(%s)'`, regexp.QuoteMeta(key), regexp.QuoteMeta(oldValue)) }, - replacement: func(newValue string) string { - return fmt.Sprintf(`'%s'`, newValue) + replacement: func(key, newValue string) string { + return fmt.Sprintf(`${1}'%s'`, newValue) }, }, { // Handles unquoted values at end of line: key: value\n name: "unquoted-eol", - pattern: func(oldValue string) string { - return fmt.Sprintf(`: (%s)(\s*)$`, regexp.QuoteMeta(oldValue)) + pattern: func(key, oldValue string) string { + return fmt.Sprintf(`(%s:\s*)(%s)(\s*)$`, regexp.QuoteMeta(key), regexp.QuoteMeta(oldValue)) }, - replacement: func(newValue string) string { - return fmt.Sprintf(`: %s$2`, newValue) + replacement: func(key, newValue string) string { + return fmt.Sprintf(`${1}%s${3}`, newValue) }, }, { // Handles unquoted values followed by inline comment: key: value # comment name: "unquoted-with-comment", - pattern: func(oldValue string) string { - return fmt.Sprintf(`: (%s)(\s*#)`, regexp.QuoteMeta(oldValue)) + pattern: func(key, oldValue string) string { + return fmt.Sprintf(`(%s:\s*)(%s)(\s*#)`, regexp.QuoteMeta(key), regexp.QuoteMeta(oldValue)) }, - replacement: func(newValue string) string { - return fmt.Sprintf(`: %s$2`, newValue) + replacement: func(key, newValue string) string { + return fmt.Sprintf(`${1}%s${3}`, newValue) }, }, } // surgicalReplace performs a targeted replacement of a YAML value while preserving -// the original formatting (quotes, whitespace, etc.) -func surgicalReplace(data []byte, oldValue, newValue string) ([]byte, error) { +// the original formatting (quotes, whitespace, etc.). The key parameter ensures +// we only replace the value for the specific YAML key being updated. +func surgicalReplace(data []byte, key, oldValue, newValue string) ([]byte, error) { content := string(data) // Try each replacement rule in order; use the first one that matches for _, rule := range replacementRules { - pattern := rule.pattern(oldValue) + pattern := rule.pattern(key, oldValue) re := regexp.MustCompile(`(?m)` + pattern) if re.MatchString(content) { - result := re.ReplaceAllString(content, rule.replacement(newValue)) + result := re.ReplaceAllString(content, rule.replacement(key, newValue)) return []byte(result), nil } } - // Fallback: simple string replacement if no pattern matched - if strings.Contains(content, oldValue) { - result := strings.Replace(content, oldValue, newValue, 1) + // Fallback: key-aware simple string replacement if no pattern matched + // Look for "key: oldValue" or "key:oldValue" patterns + keyPattern := regexp.MustCompile(fmt.Sprintf(`(%s:\s*)%s`, regexp.QuoteMeta(key), regexp.QuoteMeta(oldValue))) + if keyPattern.MatchString(content) { + result := keyPattern.ReplaceAllString(content, fmt.Sprintf(`${1}%s`, newValue)) return []byte(result), nil } - return nil, fmt.Errorf("could not find value %q to replace", oldValue) + return nil, fmt.Errorf("could not find value %q for key %q to replace", oldValue, key) } // findEmbeddedVersion looks for a version pattern in the value and returns it if found. diff --git a/internal/files/yaml_test.go b/internal/files/yaml_test.go index 6eaca72..decec67 100644 --- a/internal/files/yaml_test.go +++ b/internal/files/yaml_test.go @@ -251,6 +251,34 @@ func TestConvertToYAMLPath(t *testing.T) { } } +func TestExtractKeyFromPath(t *testing.T) { + t.Parallel() + + tests := []struct { + input string + want string + }{ + {"version", "version"}, + {"appVersion", "appVersion"}, + {"metadata.version", "version"}, + {"spec.template.spec.image.tag", "tag"}, + {"operator.image", "image"}, + {"containers[0].image", "image"}, + {"spec.containers[0].image", "image"}, + {"data[0]", "data"}, + } + + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + t.Parallel() + got := extractKeyFromPath(tt.input) + if got != tt.want { + t.Errorf("extractKeyFromPath(%q) = %q, want %q", tt.input, got, tt.want) + } + }) + } +} + func TestUpdateYAMLFile_InvalidPath(t *testing.T) { t.Parallel() @@ -607,3 +635,115 @@ app: }) } } + +// TestUpdateYAMLFile_SameValueDifferentKeys tests that updating one field does not +// accidentally modify another field with the same value but different formatting. +// This was a bug where updating "version: 0.9.0" would also modify "appVersion: "0.9.0"" +// because the replacement was not key-aware. +func TestUpdateYAMLFile_SameValueDifferentKeys(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input string + config VersionFileConfig + currentVersion string + newVersion string + wantContains []string + wantNotContain []string + }{ + { + name: "update unquoted version without affecting quoted appVersion", + input: `apiVersion: v2 +name: test-chart +version: 0.9.0 +appVersion: "0.9.0" +`, + config: VersionFileConfig{Path: "version"}, + currentVersion: "0.9.0", + newVersion: "0.9.1", + wantContains: []string{ + "version: 0.9.1", + `appVersion: "0.9.0"`, // appVersion should NOT be changed + }, + wantNotContain: []string{ + `appVersion: "0.9.1"`, // should NOT happen + }, + }, + { + name: "update quoted appVersion without affecting unquoted version", + input: `apiVersion: v2 +name: test-chart +version: 0.9.0 +appVersion: "0.9.0" +`, + config: VersionFileConfig{Path: "appVersion"}, + currentVersion: "0.9.0", + newVersion: "0.9.1", + wantContains: []string{ + "version: 0.9.0", // version should NOT be changed + `appVersion: "0.9.1"`, + }, + wantNotContain: []string{ + "version: 0.9.1", // should NOT happen + }, + }, + { + name: "sequential updates to different keys with same value", + input: `apiVersion: v2 +name: operator-crds +version: 0.9.0 +appVersion: "0.9.0" +`, + config: VersionFileConfig{Path: "version"}, + currentVersion: "0.9.0", + newVersion: "0.9.1", + wantContains: []string{ + "version: 0.9.1", + `appVersion: "0.9.0"`, + }, + }, + { + name: "different keys same value - nested paths", + input: `chart: + version: 0.9.0 + appVersion: "0.9.0" + description: A test chart +`, + config: VersionFileConfig{Path: "chart.version"}, + currentVersion: "0.9.0", + newVersion: "0.9.1", + wantContains: []string{ + "version: 0.9.1", + `appVersion: "0.9.0"`, // appVersion should NOT be changed + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + tmpPath := createTempFile(t, tt.input, "test*.yaml") + cfg := tt.config + cfg.File = tmpPath + + err := UpdateYAMLFile(cfg, tt.currentVersion, tt.newVersion) + if err != nil { + t.Fatalf("UpdateYAMLFile() error = %v", err) + } + + got := readTempFile(t, tmpPath) + for _, want := range tt.wantContains { + if !strings.Contains(got, want) { + t.Errorf("UpdateYAMLFile() should contain %q, got:\n%s", want, got) + } + } + for _, notWant := range tt.wantNotContain { + if strings.Contains(got, notWant) { + t.Errorf("UpdateYAMLFile() should NOT contain %q, got:\n%s", notWant, got) + } + } + }) + } +} From 7ee9225c41d056fc519b6f0b68b8568ca9adab5b Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Wed, 4 Feb 2026 20:14:45 +0000 Subject: [PATCH 2/2] Fix unused parameter lint errors Rename unused 'key' parameter to '_' in replacement functions. Co-Authored-By: Claude Opus 4.5 --- internal/files/yaml.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/files/yaml.go b/internal/files/yaml.go index 6daa634..d006383 100644 --- a/internal/files/yaml.go +++ b/internal/files/yaml.go @@ -142,7 +142,7 @@ var replacementRules = []replacementRule{ pattern: func(key, oldValue string) string { return fmt.Sprintf(`(%s:\s*)"(%s)"`, regexp.QuoteMeta(key), regexp.QuoteMeta(oldValue)) }, - replacement: func(key, newValue string) string { + replacement: func(_, newValue string) string { // Use ${1} syntax to avoid ambiguity when newValue starts with a digit return fmt.Sprintf(`${1}"%s"`, newValue) }, @@ -153,7 +153,7 @@ var replacementRules = []replacementRule{ pattern: func(key, oldValue string) string { return fmt.Sprintf(`(%s:\s*)'(%s)'`, regexp.QuoteMeta(key), regexp.QuoteMeta(oldValue)) }, - replacement: func(key, newValue string) string { + replacement: func(_, newValue string) string { return fmt.Sprintf(`${1}'%s'`, newValue) }, }, @@ -163,7 +163,7 @@ var replacementRules = []replacementRule{ pattern: func(key, oldValue string) string { return fmt.Sprintf(`(%s:\s*)(%s)(\s*)$`, regexp.QuoteMeta(key), regexp.QuoteMeta(oldValue)) }, - replacement: func(key, newValue string) string { + replacement: func(_, newValue string) string { return fmt.Sprintf(`${1}%s${3}`, newValue) }, }, @@ -173,7 +173,7 @@ var replacementRules = []replacementRule{ pattern: func(key, oldValue string) string { return fmt.Sprintf(`(%s:\s*)(%s)(\s*#)`, regexp.QuoteMeta(key), regexp.QuoteMeta(oldValue)) }, - replacement: func(key, newValue string) string { + replacement: func(_, newValue string) string { return fmt.Sprintf(`${1}%s${3}`, newValue) }, },