From 60960c450c371675685d5dc3848bc85482cabd06 Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Wed, 26 Aug 2026 07:32:08 -0700 Subject: [PATCH 1/9] feat(ci): bump chart versions when a service releases A service release tag resolves to the charts that declare they deploy it, and their appVersion and matching image tag move to the released version. A chart states its version twice: appVersion in Chart.yaml, and an image tag at a path that differs per chart. Measured across all 22 charts: 12 agree, 3 differ, 7 set no tag. The drift is real, not theoretical: ratelimiter reads appVersion 1.0.0 against a tag of 1.15.2, api-keys-colocated 0.0.4 against 1.5.0. So agreement is the evidence, rather than declaring one field authoritative and overwriting the other: agree both move. The shared value identifies exactly which tag lines belong to this service, so no per-chart path config is needed and other images in the same values.yaml are untouched. no tag appVersion moves alone. differ nothing moves, the chart is reported, exit 3. floating nothing moves. latest is not a pin. A bumper that guesses which of two disagreeing fields to move will eventually move the wrong one, and the result looks like a routine version bump in review. Exit 3 for a refusal, distinct from 1. A caller cannot tell the two apart from stderr, because the failure path writes there too and an unresolvable tag then reads exactly like a refused chart. After a refusal the charts that could move did; after a failure nothing did. Go rather than Python, per tools/AGENTS.md. Files are rewritten line by line rather than through a YAML marshaller, which would discard comments, key order, and quoting style across a whole file for one value. tools/ci/chart-version-bumper builds the binary rather than using `go run`: `go run` does not propagate exit status, printing "exit status 3" and exiting 1, which would collapse the refusal code into an ordinary failure. Fifteen tests, mutation tested. Ten mutants die, including bumping a drifted chart anyway, accepting a floating tag, replacing every tag line rather than only the one matching the old appVersion, aborting on a refusal instead of applying the charts that could move, and resolving a chart release tag as though it were a service. Two survived the first pass and are now covered. The longest-path tie-break was untested, because the case the test used (src/a against src/a/b) cannot reach it: a tag of src/a/b/v2.0.0 does not start with src/a/v, so only one candidate ever existed. Excluding charts when resolving a tag was untested as well. Co-authored-by: Balaji Ganesan Signed-off-by: Balaji Ganesan --- tools/chart-version-bumper/.gitignore | 2 + tools/chart-version-bumper/chart.go | 172 ++++++++++++ tools/chart-version-bumper/go.mod | 3 + tools/chart-version-bumper/main.go | 135 ++++++++++ tools/chart-version-bumper/main_test.go | 335 ++++++++++++++++++++++++ tools/chart-version-bumper/metadata.go | 89 +++++++ tools/ci/chart-version-bumper | 36 +++ 7 files changed, 772 insertions(+) create mode 100644 tools/chart-version-bumper/.gitignore create mode 100644 tools/chart-version-bumper/chart.go create mode 100644 tools/chart-version-bumper/go.mod create mode 100644 tools/chart-version-bumper/main.go create mode 100644 tools/chart-version-bumper/main_test.go create mode 100644 tools/chart-version-bumper/metadata.go create mode 100755 tools/ci/chart-version-bumper diff --git a/tools/chart-version-bumper/.gitignore b/tools/chart-version-bumper/.gitignore new file mode 100644 index 000000000..4d96b1695 --- /dev/null +++ b/tools/chart-version-bumper/.gitignore @@ -0,0 +1,2 @@ +# go build ./... drops the binary here; it must never be committed. +/chart-version-bumper diff --git a/tools/chart-version-bumper/chart.go b/tools/chart-version-bumper/chart.go new file mode 100644 index 000000000..68d92dbae --- /dev/null +++ b/tools/chart-version-bumper/chart.go @@ -0,0 +1,172 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "fmt" + "os" + "path/filepath" + "regexp" + "sort" + "strings" +) + +// Action is what a chart can do with a released version. +type Action int + +const ( + // ActionBoth moves appVersion and the image tag that agrees with it. + ActionBoth Action = iota + // ActionAppVersionOnly moves appVersion; the chart sets no image tag. + ActionAppVersionOnly + // ActionRefuse moves nothing and reports why. + ActionRefuse + // ActionSkip moves nothing because there is no chart to move. + ActionSkip +) + +// Floating tags are not pins. Replacing one with a version is a behaviour +// change rather than a bump, so a chart carrying one is refused. +var floating = map[string]bool{ + "latest": true, + "main": true, + "stable": true, + "edge": true, +} + +// Chart and values files are rewritten line by line rather than round-tripped +// through a YAML library, which would discard comments, key order, and quoting +// style across the whole file for the sake of one value. +var ( + appVersionRE = regexp.MustCompile(`(?m)^(appVersion:\s*)"?([^"\s#]+)"?(.*)$`) + tagRE = regexp.MustCompile(`(?m)^\s+tag:\s*"?([^"\s#]*)"?`) +) + +// A Plan is what to do for one chart. +type Plan struct { + Action Action + Detail string + Current string + Tags []string +} + +// ChartFiles locates a chart's Chart.yaml and values.yaml under chartPath. +// Some chart directories hold the chart directly; others nest it one level +// down. +func ChartFiles(root, chartPath string) (chartYAML, valuesYAML string) { + base := filepath.Join(root, chartPath) + candidates := []string{filepath.Join(base, "Chart.yaml")} + if nested, err := filepath.Glob(filepath.Join(base, "*", "Chart.yaml")); err == nil { + sort.Strings(nested) + candidates = append(candidates, nested...) + } + for _, c := range candidates { + if _, err := os.Stat(c); err == nil { + return c, filepath.Join(filepath.Dir(c), "values.yaml") + } + } + return "", "" +} + +// Plan decides what one chart can do with the released version. +func PlanFor(root string, chart Entry, version string) (Plan, error) { + chartYAML, valuesYAML := ChartFiles(root, chart.Path) + if chartYAML == "" { + return Plan{Action: ActionSkip, Detail: fmt.Sprintf("no Chart.yaml under %s", chart.Path)}, nil + } + + b, err := os.ReadFile(chartYAML) + if err != nil { + return Plan{}, fmt.Errorf("read %s: %w", chartYAML, err) + } + m := appVersionRE.FindStringSubmatch(string(b)) + if m == nil { + return Plan{Action: ActionSkip, Detail: "chart declares no appVersion"}, nil + } + current := m[2] + + var tags []string + if vb, err := os.ReadFile(valuesYAML); err == nil { + for _, hit := range tagRE.FindAllStringSubmatch(string(vb), -1) { + if hit[1] != "" { + tags = append(tags, hit[1]) + } + } + } else if !os.IsNotExist(err) { + return Plan{}, fmt.Errorf("read %s: %w", valuesYAML, err) + } + + var floats []string + for _, t := range tags { + if floating[t] { + floats = append(floats, t) + } + } + if len(floats) > 0 { + return Plan{ActionRefuse, "image tag is floating (" + strings.Join(floats, ", ") + ")", current, tags}, nil + } + if len(tags) == 0 { + return Plan{ActionAppVersionOnly, "no image tag set", current, tags}, nil + } + for _, t := range tags { + if t == current { + return Plan{ActionBoth, "appVersion and image tag agree", current, tags}, nil + } + } + return Plan{ + ActionRefuse, + fmt.Sprintf("appVersion %s does not match image tag(s) %s", current, strings.Join(tags, ", ")), + current, + tags, + }, nil +} + +// Apply writes the planned change for one chart. +func Apply(root string, chart Entry, version string, p Plan) error { + chartYAML, valuesYAML := ChartFiles(root, chart.Path) + b, err := os.ReadFile(chartYAML) + if err != nil { + return fmt.Errorf("read %s: %w", chartYAML, err) + } + text := string(b) + current := appVersionRE.FindStringSubmatch(text)[2] + + replaced := false + updated := appVersionRE.ReplaceAllStringFunc(text, func(line string) string { + if replaced { + return line + } + replaced = true + g := appVersionRE.FindStringSubmatch(line) + return fmt.Sprintf("%s%q%s", g[1], version, g[3]) + }) + if err := writeFilePreservingMode(chartYAML, updated); err != nil { + return err + } + + if p.Action != ActionBoth { + return nil + } + // Replace only tag lines holding the value appVersion also held. Any other + // tag in this file belongs to a different image, and moving it would point + // a sidecar at a version that was never built for it. + vb, err := os.ReadFile(valuesYAML) + if err != nil { + return fmt.Errorf("read %s: %w", valuesYAML, err) + } + matching := regexp.MustCompile(`(?m)^(\s+tag:\s*)"?` + regexp.QuoteMeta(current) + `"?(\s*(?:#.*)?)$`) + out := matching.ReplaceAllString(string(vb), fmt.Sprintf(`${1}"%s"${2}`, version)) + return writeFilePreservingMode(valuesYAML, out) +} + +func writeFilePreservingMode(path, content string) error { + mode := os.FileMode(0o644) + if fi, err := os.Stat(path); err == nil { + mode = fi.Mode().Perm() + } + if err := os.WriteFile(path, []byte(content), mode); err != nil { + return fmt.Errorf("write %s: %w", path, err) + } + return nil +} diff --git a/tools/chart-version-bumper/go.mod b/tools/chart-version-bumper/go.mod new file mode 100644 index 000000000..3b4a7850d --- /dev/null +++ b/tools/chart-version-bumper/go.mod @@ -0,0 +1,3 @@ +module chart-version-bumper + +go 1.26 diff --git a/tools/chart-version-bumper/main.go b/tools/chart-version-bumper/main.go new file mode 100644 index 000000000..4dde701b6 --- /dev/null +++ b/tools/chart-version-bumper/main.go @@ -0,0 +1,135 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Command chart-version-bumper moves a chart's version fields to a newly +// released service version. +// +// chart-version-bumper --tag src/control-plane-services/notary/v1.9.0 [--write] +// +// This is the first hop of the service to chart to stack cascade. A service +// releases, the charts that deploy it move their appVersion and image tag, and +// the chart release that follows is what stack-pin-resolver then acts on. +// +// Which charts deploy the service comes from the deploys list in +// tools/ci/github-release-subprojects.json; see chart-service-edge. +// +// Which field to move is the awkward part, because a chart states its version +// twice. Chart.yaml has appVersion at a fixed location. values.yaml has an image +// tag at a path that differs per chart, and the two have drifted apart in real +// charts: api-keys-colocated reads 0.0.4 against a tag of 1.5.0, ratelimiter +// 1.0.0 against 1.15.2. +// +// Rather than declare one of them authoritative and silently overwrite the +// other, this uses their agreement as the evidence: +// +// they agree both move together. The current value identifies exactly +// which tag lines belong to this service, so no per-chart +// path configuration is needed. +// no tag is set appVersion moves alone. The chart resolves its image from +// Chart.AppVersion or the operator supplies a tag. +// they differ nothing moves, and the chart is reported. Someone chose +// that split, or it is a bug; either way it is not something +// to resolve by fiat during an automated bump. +// the tag floats nothing moves. A tag of latest is not a pin, and replacing +// it with a version would be a behaviour change rather than +// a bump. +// +// The refusals are the point. A bumper that guesses which of two disagreeing +// fields to move will eventually move the wrong one, and the result looks like +// a routine version bump in review. +// +// Exit codes: 0 nothing to report, 3 at least one chart refused (the rest were +// still applied), anything else a failure that applied nothing. +package main + +import ( + "flag" + "fmt" + "io" + "os" +) + +// RefusedExit is returned when at least one chart refused the bump. +// +// Its own code, because a caller has to tell a refusal from a failure and +// cannot do it by looking at stderr: an unresolvable tag writes there too, so +// it would read exactly like a refused chart. A refusal means the charts that +// could move did move; a failure means nothing did. 3 rather than 2 because +// flag parsing already exits 2 on a usage error. +const RefusedExit = 3 + +func main() { + tag := flag.String("tag", "", "service release tag, for example src/control-plane-services/notary/v1.9.0") + write := flag.Bool("write", false, "apply the changes rather than only reporting them") + root := flag.String("root", ".", "repository root") + flag.Parse() + + if *tag == "" { + fmt.Fprintln(os.Stderr, "error: --tag is required") + flag.Usage() + os.Exit(2) + } + + code, err := Run(*root, *tag, *write, os.Stdout, os.Stderr) + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } + os.Exit(code) +} + +// Run resolves the tag, plans every chart that deploys the released service, +// and applies the ones that can move safely. +func Run(root, tag string, write bool, out, errOut io.Writer) (int, error) { + meta, err := LoadMetadata(root) + if err != nil { + return 1, err + } + + serviceID, version, err := meta.ServiceForTag(tag) + if err != nil { + return 1, err + } + charts := meta.ChartsDeploying(serviceID) + fmt.Fprintf(out, "%s -> service %s, version %s\n", tag, serviceID, version) + + if len(charts) == 0 { + // Not an error. Plenty of services ship no chart, and chart-service-edge + // is what reports charts that have not declared an edge yet. + fmt.Fprintf(out, "no chart declares that it deploys %s; nothing to do\n", serviceID) + return 0, nil + } + + refused := 0 + for _, chart := range charts { + p, err := PlanFor(root, chart, version) + if err != nil { + return 1, err + } + switch p.Action { + case ActionRefuse: + fmt.Fprintf(errOut, " %s: REFUSED, %s\n", chart.ID, p.Detail) + refused++ + case ActionSkip: + fmt.Fprintf(out, " %s: skipped, %s\n", chart.ID, p.Detail) + default: + if p.Current == version { + fmt.Fprintf(out, " %s: already %s\n", chart.ID, version) + continue + } + fmt.Fprintf(out, " %s: %s -> %s (%s)\n", chart.ID, p.Current, version, p.Detail) + if write { + if err := Apply(root, chart, version, p); err != nil { + return 1, err + } + } + } + } + + // A refusal is a real finding: the chart wanted a bump and could not take + // one safely. Exiting non-zero puts it in front of a person. + if refused > 0 { + return RefusedExit, nil + } + return 0, nil +} diff --git a/tools/chart-version-bumper/main_test.go b/tools/chart-version-bumper/main_test.go new file mode 100644 index 000000000..2d0b6186d --- /dev/null +++ b/tools/chart-version-bumper/main_test.go @@ -0,0 +1,335 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "fmt" + "os" + "path/filepath" + "strings" + "testing" +) + +// The dangerous direction is a bump that lands on the wrong line. A chart often +// names several images, and only the one whose tag matches appVersion belongs +// to the service that just released. The multi-image fixture below is the case +// that would expose a substitution that is merely "close enough". + +type fixture struct{ root string } + +func newFixture(t *testing.T, metadata string) *fixture { + t.Helper() + f := &fixture{root: t.TempDir()} + if err := os.MkdirAll(filepath.Join(f.root, "tools", "ci"), 0o755); err != nil { + t.Fatal(err) + } + f.metadata(t, metadata) + return f +} + +func (f *fixture) metadata(t *testing.T, body string) { + t.Helper() + if err := os.WriteFile(filepath.Join(f.root, MetadataPath), []byte(body), 0o644); err != nil { + t.Fatal(err) + } +} + +func (f *fixture) chart(t *testing.T, id, appVersion, values string) { + t.Helper() + dir := filepath.Join(f.root, "deploy", "helm", id) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + chart := fmt.Sprintf("apiVersion: v2\nname: helm-%s\nversion: 0.0.0\nappVersion: %q\n", id, appVersion) + if err := os.WriteFile(filepath.Join(dir, "Chart.yaml"), []byte(chart), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "values.yaml"), []byte(values+"\n"), 0o644); err != nil { + t.Fatal(err) + } +} + +func (f *fixture) run(t *testing.T, tag string, write bool) (int, string, string) { + t.Helper() + var out, errOut bytes.Buffer + code, err := Run(f.root, tag, write, &out, &errOut) + if err != nil { + // A returned error is the failure path; surface it on stderr the way the + // command does so tests can assert on the message. + fmt.Fprintln(&errOut, err) + } + return code, out.String(), errOut.String() +} + +func (f *fixture) read(t *testing.T, id, name string) string { + t.Helper() + b, err := os.ReadFile(filepath.Join(f.root, "deploy", "helm", id, name)) + if err != nil { + t.Fatal(err) + } + return string(b) +} + +const meta = `{"services":[ + {"id":"svc","path":"src/svc"}, + {"id":"other","path":"src/other"}, + {"id":"agree","path":"deploy/helm/agree","deploys":["svc"]}, + {"id":"drift","path":"deploy/helm/drift","deploys":["svc"]}, + {"id":"notag","path":"deploy/helm/notag","deploys":["svc"]}, + {"id":"floater","path":"deploy/helm/floater","deploys":["svc"]}, + {"id":"multi","path":"deploy/helm/multi","deploys":["svc"]}, + {"id":"unrelated","path":"deploy/helm/unrelated","deploys":["other"]} +]}` + +func full(t *testing.T) *fixture { + f := newFixture(t, meta) + f.chart(t, "agree", "1.0.0", "image:\n tag: \"1.0.0\"") + f.chart(t, "drift", "1.0.0", "image:\n tag: \"9.9.9\"") + f.chart(t, "notag", "1.0.0", "image:\n repository: \"\"") + f.chart(t, "floater", "1.0.0", "image:\n tag: \"latest\"") + // Two images. Only the one matching appVersion belongs to this service. + f.chart(t, "multi", "1.0.0", "app:\n image:\n tag: \"1.0.0\"\nsidecar:\n image:\n tag: \"3.3.3\"") + f.chart(t, "unrelated", "1.0.0", "image:\n tag: \"1.0.0\"") + return f +} + +func TestDriftedChartRefusesWithTheRefusalCode(t *testing.T) { + // Exit 3, not any non-zero: a caller has to tell a refusal (charts that + // could move did) from a failure (nothing moved), and cannot do it by + // looking at stderr because the failure path writes there too. The unowned + // tag test below pins the other side of that distinction. + code, _, errOut := full(t).run(t, "src/svc/v2.0.0", false) + if code != RefusedExit { + t.Fatalf("want exit %d, got %d", RefusedExit, code) + } + if !strings.Contains(errOut, "does not match image tag") { + t.Fatalf("refusal must name the mismatch:\n%s", errOut) + } +} + +func TestFloatingTagRefusesForBeingFloating(t *testing.T) { + // Asserted on the reason, not on the word "floating": name the fixture + // chart "floating" and a substring check passes on the chart id even with + // the floating check removed entirely. Mutation testing caught exactly that + // in the first version of this suite, which is why the chart is "floater". + _, _, errOut := full(t).run(t, "src/svc/v2.0.0", false) + if !strings.Contains(errOut, "image tag is floating") { + t.Fatalf("a floating tag must be refused for being floating:\n%s", errOut) + } +} + +func TestAgreeingChartIsPlanned(t *testing.T) { + _, out, _ := full(t).run(t, "src/svc/v2.0.0", false) + if !strings.Contains(out, "agree: 1.0.0 -> 2.0.0 (appVersion and image tag agree)") { + t.Fatalf("agreeing chart should plan a bump:\n%s", out) + } +} + +func TestChartWithNoTagMovesAppVersionOnly(t *testing.T) { + _, out, _ := full(t).run(t, "src/svc/v2.0.0", false) + if !strings.Contains(out, "notag: 1.0.0 -> 2.0.0 (no image tag set)") { + t.Fatalf("a chart with no tag should move appVersion alone:\n%s", out) + } +} + +func TestUnrelatedChartIsNotTouched(t *testing.T) { + // It declares a different service. Reaching it would mean the deploys edge + // is not actually gating anything. + _, out, errOut := full(t).run(t, "src/svc/v2.0.0", false) + if strings.Contains(out, "unrelated") || strings.Contains(errOut, "unrelated") { + t.Fatalf("a chart deploying another service must not be considered:\n%s%s", out, errOut) + } +} + +func TestUnownedTagIsAFailureDistinctFromARefusal(t *testing.T) { + code, _, errOut := full(t).run(t, "src/nosuch/v1.0.0", false) + if code != 1 { + t.Fatalf("an unowned tag must fail with 1, not %d", code) + } + if code == RefusedExit { + t.Fatal("a failure must not share the refusal exit code") + } + if !strings.Contains(errOut, "no service in release metadata owns the tag") { + t.Fatalf("failure should name the problem:\n%s", errOut) + } +} + +func TestServiceWithNoChartIsNotAnError(t *testing.T) { + f := newFixture(t, `{"services":[{"id":"lonely","path":"src/lonely"}]}`) + code, out, _ := f.run(t, "src/lonely/v1.0.0", false) + if code != 0 { + t.Fatalf("a service shipping no chart is not an error, got %d", code) + } + if !strings.Contains(out, "nothing to do") { + t.Fatalf("it should say so:\n%s", out) + } +} + +func TestLongestPathWinsWhenTwoServicePathsBothMatch(t *testing.T) { + // Both paths below genuinely match the tag, which is what makes the tie-break + // reachable: "src/a/v1/v2.0.0" starts with "src/a" + "/v" and with + // "src/a/v1" + "/v". Picking the shorter one attributes the release to the + // wrong service and reads the version as "1/v2.0.0". + // + // The obvious nesting case, "src/a" against "src/a/b", cannot exercise this: + // a tag of "src/a/b/v2.0.0" does not start with "src/a/v", so only one + // candidate ever exists and the tie-break is never consulted. + f := newFixture(t, `{"services":[ + {"id":"outer","path":"src/a"}, + {"id":"inner","path":"src/a/v1"}, + {"id":"c","path":"deploy/helm/c","deploys":["inner"]} + ]}`) + f.chart(t, "c", "1.0.0", "image:\n tag: \"1.0.0\"") + _, out, _ := f.run(t, "src/a/v1/v2.0.0", false) + if !strings.Contains(out, "service inner, version 2.0.0") { + t.Fatalf("the longest matching path must win:\n%s", out) + } +} + +func TestChartReleaseTagIsNotAServiceRelease(t *testing.T) { + // Chart entries are excluded when resolving a tag. Without that, a chart + // release resolves to the chart itself as though it were a service, finds no + // chart deploying it, and reports "nothing to do" with exit 0. A chart + // release is stack-pin-resolver's business, and silently succeeding here + // would hide that it reached the wrong tool. + f := newFixture(t, `{"services":[ + {"id":"svc","path":"src/svc"}, + {"id":"c","path":"deploy/helm/c","deploys":["svc"]} + ]}`) + f.chart(t, "c", "1.0.0", "image:\n tag: \"1.0.0\"") + code, out, errOut := f.run(t, "deploy/helm/c/v1.2.0", false) + if code != 1 { + t.Fatalf("a chart release tag must not resolve to a service, got exit %d:\n%s", code, out) + } + if !strings.Contains(errOut, "no service in release metadata owns the tag") { + t.Fatalf("it should say the tag owns no service:\n%s", errOut) + } +} + +func TestWriteMovesAppVersionAndOnlyTheMatchingTag(t *testing.T) { + f := newFixture(t, `{"services":[ + {"id":"svc","path":"src/svc"}, + {"id":"multi","path":"deploy/helm/multi","deploys":["svc"]} + ]}`) + f.chart(t, "multi", "1.0.0", "app:\n image:\n tag: \"1.0.0\"\nsidecar:\n image:\n tag: \"3.3.3\"") + + if code, _, errOut := f.run(t, "src/svc/v2.0.0", true); code != 0 { + t.Fatalf("write should succeed, got %d: %s", code, errOut) + } + if got := f.read(t, "multi", "Chart.yaml"); !strings.Contains(got, `appVersion: "2.0.0"`) { + t.Fatalf("appVersion did not move:\n%s", got) + } + values := f.read(t, "multi", "values.yaml") + if !strings.Contains(values, ` tag: "2.0.0"`) { + t.Fatalf("the matching image tag did not move:\n%s", values) + } + if !strings.Contains(values, ` tag: "3.3.3"`) { + t.Fatalf("the sidecar tag must be left alone, it belongs to another image:\n%s", values) + } +} + +func TestWritePreservesSurroundingContent(t *testing.T) { + // Rewriting line by line rather than round-tripping YAML is deliberate; this + // is the test that fails if someone swaps in a marshaller. + f := newFixture(t, `{"services":[ + {"id":"svc","path":"src/svc"}, + {"id":"c","path":"deploy/helm/c","deploys":["svc"]} + ]}`) + f.chart(t, "c", "1.0.0", "# a comment that must survive\nimage:\n tag: \"1.0.0\" # trailing note\n pullPolicy: IfNotPresent") + + if code, _, errOut := f.run(t, "src/svc/v2.0.0", true); code != 0 { + t.Fatalf("write should succeed, got %d: %s", code, errOut) + } + values := f.read(t, "c", "values.yaml") + for _, want := range []string{"# a comment that must survive", "pullPolicy: IfNotPresent", `tag: "2.0.0" # trailing note`} { + if !strings.Contains(values, want) { + t.Fatalf("lost %q from values.yaml:\n%s", want, values) + } + } +} + +func TestRerunningAnAppliedBumpIsANoOp(t *testing.T) { + f := newFixture(t, `{"services":[ + {"id":"svc","path":"src/svc"}, + {"id":"c","path":"deploy/helm/c","deploys":["svc"]} + ]}`) + f.chart(t, "c", "1.0.0", "image:\n tag: \"1.0.0\"") + f.run(t, "src/svc/v2.0.0", true) + before := f.read(t, "c", "values.yaml") + + code, out, _ := f.run(t, "src/svc/v2.0.0", true) + if code != 0 { + t.Fatalf("a repeat run should be clean, got %d", code) + } + if !strings.Contains(out, "already 2.0.0") { + t.Fatalf("it should say the chart is already there:\n%s", out) + } + if after := f.read(t, "c", "values.yaml"); after != before { + t.Fatalf("a repeat run rewrote the file:\n%s\n---\n%s", before, after) + } +} + +func TestRefusalStillAppliesTheChartsThatCouldMove(t *testing.T) { + // This is what the refusal exit code buys: partial progress must survive. + // If a refusal aborted the run, the agreeing chart would never move. + f := full(t) + code, _, _ := f.run(t, "src/svc/v2.0.0", true) + if code != RefusedExit { + t.Fatalf("want refusal exit, got %d", code) + } + if got := f.read(t, "agree", "Chart.yaml"); !strings.Contains(got, `appVersion: "2.0.0"`) { + t.Fatalf("the agreeing chart should still have moved:\n%s", got) + } + if got := f.read(t, "drift", "Chart.yaml"); !strings.Contains(got, `appVersion: "1.0.0"`) { + t.Fatalf("the drifted chart must not have moved:\n%s", got) + } + if got := f.read(t, "floater", "values.yaml"); !strings.Contains(got, `tag: "latest"`) { + t.Fatalf("the floating tag must not have been replaced:\n%s", got) + } +} + +func TestRealChartsResolve(t *testing.T) { + // Against the checked-in metadata and charts, so a chart directory layout + // this tool cannot read shows up here rather than during a release. + root := repoRoot(t) + m, err := LoadMetadata(root) + if err != nil { + t.Fatalf("checked-in release metadata does not load: %v", err) + } + seen := 0 + for _, e := range m.Services { + if !strings.HasPrefix(e.Path, ChartPrefix) || e.Deploys == nil { + continue + } + chartYAML, _ := ChartFiles(root, e.Path) + if chartYAML == "" { + t.Errorf("chart %s declares an edge but no Chart.yaml was found under %s", e.ID, e.Path) + continue + } + if _, err := PlanFor(root, e, "9.9.9"); err != nil { + t.Errorf("planning %s failed: %v", e.ID, err) + } + seen++ + } + if seen == 0 { + t.Fatal("no chart with a declared edge was found; the metadata path or prefix is wrong") + } +} + +func repoRoot(t *testing.T) string { + t.Helper() + dir, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + for i := 0; i < 6; i++ { + if _, err := os.Stat(filepath.Join(dir, MetadataPath)); err == nil { + return dir + } + dir = filepath.Dir(dir) + } + t.Fatalf("could not find %s above the test directory", MetadataPath) + return "" +} diff --git a/tools/chart-version-bumper/metadata.go b/tools/chart-version-bumper/metadata.go new file mode 100644 index 000000000..4517f4ae7 --- /dev/null +++ b/tools/chart-version-bumper/metadata.go @@ -0,0 +1,89 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "strings" +) + +// MetadataPath is the release metadata that github-release already owns. +const MetadataPath = "tools/ci/github-release-subprojects.json" + +// ChartPrefix marks an entry as a chart rather than a service. +const ChartPrefix = "deploy/helm/" + +// Entry is one subproject in the release metadata. +type Entry struct { + ID string `json:"id"` + Path string `json:"path"` + Deploys []string `json:"deploys"` +} + +// Metadata is the decoded release metadata file. +type Metadata struct { + Services []Entry `json:"services"` +} + +// LoadMetadata reads and decodes the release metadata under root. +func LoadMetadata(root string) (*Metadata, error) { + path := filepath.Join(root, MetadataPath) + b, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("read release metadata: %w", err) + } + var m Metadata + if err := json.Unmarshal(b, &m); err != nil { + return nil, fmt.Errorf("parse %s: %w", path, err) + } + return &m, nil +} + +// ServiceForTag maps a release tag to the service that owns it and the version +// the tag carries. +// +// The released tag carries the version, so there is no "newest version" lookup +// and none of the ordering questions that come with one. +func (m *Metadata) ServiceForTag(tag string) (serviceID, version string, err error) { + bestPath, bestID := "", "" + for _, e := range m.Services { + // Charts are excluded: a chart release is stack-pin-resolver's business, + // and a chart path could otherwise shadow the service it deploys. + if e.Path == "" || strings.HasPrefix(e.Path, ChartPrefix) { + continue + } + if !strings.HasPrefix(tag, e.Path+"/v") { + continue + } + // Longest match wins: subtree paths nest, so a shorter path can be a + // prefix of the one that actually owns the tag. + if bestPath == "" || len(e.Path) > len(bestPath) { + bestPath, bestID = e.Path, e.ID + } + } + if bestPath == "" { + return "", "", fmt.Errorf("no service in release metadata owns the tag %s", tag) + } + return bestID, tag[len(bestPath)+2:], nil +} + +// ChartsDeploying returns the chart entries that declare they deploy serviceID. +func (m *Metadata) ChartsDeploying(serviceID string) []Entry { + var out []Entry + for _, e := range m.Services { + if !strings.HasPrefix(e.Path, ChartPrefix) { + continue + } + for _, s := range e.Deploys { + if s == serviceID { + out = append(out, e) + break + } + } + } + return out +} diff --git a/tools/ci/chart-version-bumper b/tools/ci/chart-version-bumper new file mode 100755 index 000000000..d2f9c4a9d --- /dev/null +++ b/tools/ci/chart-version-bumper @@ -0,0 +1,36 @@ +#!/usr/bin/env bash +# SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Stable CI entrypoint for the Go tool in tools/chart-version-bumper. +# +# The wrapper exists for two reasons. +# +# The repository root. `go run -C ` leaves the process running with that +# directory as its working directory, so the tool cannot find deploy/helm or the +# release metadata on its own. Resolving the root from this script's own +# location means callers do not have to pass it. +# +# The exit code. `go run` does NOT propagate the program's status: it prints +# "exit status 3" and exits 1, collapsing every non-zero code into one. This +# tool's exit 3 means "some charts refused, the rest were applied" and is +# distinct from a failure that applied nothing, so that difference has to +# survive. Building and running the binary preserves it; go build is cached, so +# repeat invocations cost about the same as go run. +# +# Run the tests with: go test -C tools/chart-version-bumper ./... +set -euo pipefail + +repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +bin_dir="$(mktemp -d)" +trap 'rm -rf "${bin_dir}"' EXIT + +go build -C "${repo_root}/tools/chart-version-bumper" -o "${bin_dir}/chart-version-bumper" . + +# Not exec, so the trap above still runs, and not under errexit, so the exit +# code reaches the caller rather than aborting the shell first. +set +e +"${bin_dir}/chart-version-bumper" --root "${repo_root}" "$@" +status=$? +set -e +exit "${status}" From 4c7ecfe7da53c2d9105b02e21c60699dac304d21 Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Wed, 26 Aug 2026 07:32:08 -0700 Subject: [PATCH 2/9] ci(charts): open a version bump PR on a service release Drives chart-version-bumper from the release event, onto a fixed branch so several releases landing close together produce one pull request rather than a pile that conflict with each other. Four things the wiring has to get right, each found by running the step body locally under `bash -e`, which is how GitHub invokes it: The step runs with `set +e`. errexit is set at invocation, and `set -uo pipefail` does not clear it, so the shell aborted on the refusal exit the bumper is designed to return, discarding the charts it had already applied safely. It branches on the exit code, not on whether stderr is empty. The failure path writes there as well, so the emptiness test classified an unresolvable tag as a refused chart and reported it under the wrong name. Change detection is scoped to the paths the commit stages. Repo-wide, any unrelated modification in the workspace sets changed=true and the commit then aborts with nothing staged. Checkout takes the default branch rather than the release event's default of the tagged commit, since the pull request targets main and bumping the tag's tree would carry a stale chart onto a branch cut from today's main. setup-go derives its version from tools/go-toolchain/go.mod; tools/ci/check-go-version fails any workflow that pins a literal. Merging the pull request does not move the stack. A chart version reaches the stack only once the chart itself is released, and publishing that release is what triggers stack-pin-bump.yml. Cutting the chart release stays a human decision. Co-authored-by: Balaji Ganesan Signed-off-by: Balaji Ganesan --- .github/workflows/chart-version-bump.yml | 218 +++++++++++++++++++++++ 1 file changed, 218 insertions(+) create mode 100644 .github/workflows/chart-version-bump.yml diff --git a/.github/workflows/chart-version-bump.yml b/.github/workflows/chart-version-bump.yml new file mode 100644 index 000000000..57ba93c03 --- /dev/null +++ b/.github/workflows/chart-version-bump.yml @@ -0,0 +1,218 @@ +# SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# When a service release is published, open a pull request moving the charts +# that deploy it to that version. +# +# This is the first hop of the service to chart to stack cascade. The second is +# stack-pin-bump.yml: merging this pull request does not by itself move the +# stack, because a chart version only reaches the stack once the chart is +# released. Cutting that chart release stays a human decision, and publishing +# it is what triggers the stack bump. +# +# Which charts deploy the released service is declared, not derived; see +# tools/ci/chart-service-edge for why deriving it is wrong. A service whose +# charts have not declared the edge yet bumps nothing and says so. + +name: chart version bump + +on: + release: + types: [published] + # Manual entry point for re-running a release whose bump did not land, and + # for exercising the job without cutting a tag. + workflow_dispatch: + inputs: + tag: + description: >- + Service release tag, for example + src/control-plane-services/notary/v1.9.0 + required: true + +permissions: + contents: read + +concurrency: + # One bump at a time. Several releases landing together refresh the same + # pull request rather than racing on the same files. + group: chart-version-bump + cancel-in-progress: false + +jobs: + bump: + runs-on: ubuntu-latest + permissions: + contents: write + pull-requests: write + steps: + - uses: actions/checkout@v4 + with: + # The default for a release event is the tagged commit, but the pull + # request targets the default branch. Bumping the tag's tree would + # carry whatever the chart looked like then onto a branch cut from + # today's main. + ref: ${{ github.event.repository.default_branch }} + + - uses: actions/setup-go@v5 + with: + # Derived from the anchor, never a literal: tools/ci/check-go-version + # fails any workflow that pins one. + go-version-file: tools/go-toolchain/go.mod + + - name: Test the bumper + # The bumper rewrites version fields in shipped charts, so its tests run + # here rather than somewhere that might not be reached. A test that + # gates nothing is not a test. + run: go test -C tools/chart-version-bumper ./... + + - name: Select the tag + id: tag + run: | + set -euo pipefail + tag="${{ github.event.inputs.tag || github.event.release.tag_name }}" + echo "tag=${tag}" >> "${GITHUB_OUTPUT}" + # Chart releases move stack pins, which is stack-pin-bump.yml's job. + # Everything else is a service release and is this job's business: + # a tag that names no known service fails below rather than here, so + # a service missing from the release metadata is visible. + case "${tag}" in + deploy/helm/*/v*) echo "applies=false" >> "${GITHUB_OUTPUT}" + echo "${tag} is a chart release; stack-pin-bump.yml handles it" ;; + */v*) echo "applies=true" >> "${GITHUB_OUTPUT}" ;; + *) echo "applies=false" >> "${GITHUB_OUTPUT}" + echo "${tag} is not a subtree release tag; nothing to do" ;; + esac + + - name: Check the chart to service edges + if: steps.tag.outputs.applies == 'true' + # Report-only. Runs here so the undeclared charts are listed in the same + # log as a bump that reached fewer charts than someone expected. + run: tools/ci/chart-service-edge --audit + + - name: Apply the bump + id: bump + if: steps.tag.outputs.applies == 'true' + run: | + set +e + set -uo pipefail + # +e, deliberately. GitHub invokes this as `bash -e`, and the bumper + # exits 3 when a chart's appVersion and image tag disagree even though + # it still applied every chart it could move safely. Aborting here + # would throw those away and leave the refusal as the only outcome. + tools/ci/chart-version-bumper \ + --tag "${{ steps.tag.outputs.tag }}" --write 2>/tmp/refusals + status=$? + cat /tmp/refusals >&2 + # The exit code, not whether stderr is empty. SystemExit writes its + # message to stderr too, so an unresolvable tag looks exactly like a + # refused chart there. 3 means the charts that could move did; any + # other non-zero means nothing moved and the run should stop. + if [ "${status}" -ne 0 ] && [ "${status}" -ne 3 ]; then + echo "bumper failed (exit ${status}); no chart was changed" >&2 + exit "${status}" + fi + if [ "${status}" -eq 3 ]; then + { + echo "refused<> "${GITHUB_OUTPUT}" + fi + # Scoped to the same paths the commit below stages. Repo-wide, any + # unrelated modification in the workspace would set changed=true and + # the commit would then abort with nothing staged. + if git diff --quiet -- deploy/helm; then + echo "changed=false" >> "${GITHUB_OUTPUT}" + echo "no chart moved" + else + echo "changed=true" >> "${GITHUB_OUTPUT}" + git --no-pager diff --stat -- deploy/helm + fi + + - name: Open or refresh the pull request + if: steps.bump.outputs.changed == 'true' + env: + GH_TOKEN: ${{ secrets.NV_GITHUB_TOKEN || github.token }} + TAG: ${{ steps.tag.outputs.tag }} + REFUSED: ${{ steps.bump.outputs.refused }} + run: | + set -euo pipefail + branch="chore/chart-version-bumps" + git config user.name "nvcf-release-bot" + git config user.email "svc-nvcf-release@nvidia.com" + + # A fixed branch, refreshed. Several releases landing close together + # then produce one pull request carrying all of their bumps instead + # of a pile that conflict with each other. + git fetch origin "${branch}" || true + if git rev-parse --verify -q "origin/${branch}" >/dev/null; then + git stash push --quiet + git checkout -B "${branch}" "origin/${branch}" + git stash pop --quiet || true + else + git checkout -B "${branch}" + fi + + git add deploy/helm + # Separate -m flags rather than an embedded multi-line string: the + # continuation lines of one would have to sit at column zero, which + # ends the YAML block scalar this script lives in. + git commit \ + -m "chore(charts): bump for ${TAG}" \ + -m "Opened by the chart version bump workflow on release of ${TAG}." \ + -m "Co-authored-by: Balaji Ganesan " + git push --force-with-lease origin "${branch}" + + notes="" + if [ -n "${REFUSED}" ]; then + notes="$(printf '%s\n' \ + "" \ + "Some charts were not bumped:" \ + "" \ + '```' \ + "${REFUSED}" \ + '```' \ + "" \ + "A chart is refused when its \`appVersion\` and image tag disagree, or when the tag is floating. Reconciling those two fields is a decision, so it is left to a person rather than resolved during an automated bump.")" + fi + + body="$(printf '%s\n' \ + "Opened by \`.github/workflows/chart-version-bump.yml\` when \`${TAG}\` was published." \ + "" \ + "The released tag carries the version, so this is a direct update rather than a lookup of the newest published image." \ + "" \ + "Merging this does not move the self-managed stack. A chart version reaches the stack only once the chart itself is released, and publishing that chart release is what triggers \`stack-pin-bump.yml\`." \ + "" \ + "Release notes: ${{ github.server_url }}/${{ github.repository }}/releases/tag/${TAG}" \ + "${notes}" \ + "" \ + "If this pull request sits unmerged, later service releases add their bumps to the same branch, so merging it applies all of them." \ + "" \ + "Github commit:" \ + "chore(charts): bump chart versions for released services" \ + "" \ + "Co-authored-by: Balaji Ganesan ")" + + if gh pr view "${branch}" --json number >/dev/null 2>&1; then + gh pr edit "${branch}" --body "${body}" + echo "refreshed the existing pull request" + else + gh pr create --base main --head "${branch}" \ + --title "chore(charts): bump chart versions for released services" \ + --body "${body}" + fi + + - name: Surface refusals + if: steps.bump.outputs.refused != '' + # Last, so it does not stop the safe bumps from being opened. A chart + # that wanted a bump and could not take one is a finding, and a green + # run would bury it. + env: + # Via env, not ${{ }} interpolation: expanding it into the script body + # is the standard Actions injection shape, even for text this + # repository produced. + REFUSED: ${{ steps.bump.outputs.refused }} + run: | + echo "::error::charts refused the bump:" + printf '%s\n' "${REFUSED}" + exit 1 From ad1154fc01e8c0022135fb3f95f1c2f5473d7054 Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Wed, 26 Aug 2026 10:25:39 -0700 Subject: [PATCH 3/9] fix(ci): address review findings on the chart version bump workflow Same four findings as the stack pin bump workflow, which shares this shape. The bump was applied on the default branch and then stashed across a checkout of the pull request branch, with `stash pop || true`. Whenever the branch already carried a bump for the same chart, that conflicts, and the swallowed failure either drops the earlier bump or commits conflict markers. The branch is now checked out before the bump runs, which also makes the run idempotent: the bumper sees the current appVersion and reports "already ". The release tag was expanded into the run: body through `${{ }}`, the standard Actions injection shape for a value an outside contributor can choose. It is passed through env, as the refusal text already was. Generated commits carried a fixed `Co-authored-by` trailer naming one person, so every future automated bump would be attributed to them in published history. Removed. The committer identity is now the standard github-actions[bot] rather than a private service account. `gh pr edit` fails against this repository with "Projects (classic) is being deprecated ... (repository.pullRequest.projectCards)", so refreshing an existing pull request would have failed on every run after the first. Replaced with the REST endpoint. The step body was re-run locally under `bash -e` across a refusing service, a clean one and an unowned tag; refusals still capture without aborting, and a failure still stops the run. Co-authored-by: Balaji Ganesan Signed-off-by: Balaji Ganesan --- .github/workflows/chart-version-bump.yml | 67 +++++++++++++++--------- 1 file changed, 43 insertions(+), 24 deletions(-) diff --git a/.github/workflows/chart-version-bump.yml b/.github/workflows/chart-version-bump.yml index 57ba93c03..985132c08 100644 --- a/.github/workflows/chart-version-bump.yml +++ b/.github/workflows/chart-version-bump.yml @@ -89,9 +89,36 @@ jobs: # log as a bump that reached fewer charts than someone expected. run: tools/ci/chart-service-edge --audit + - name: Check out the bump branch + if: steps.tag.outputs.applies == 'true' + env: + BRANCH: chore/chart-version-bumps + run: | + set -euo pipefail + # The bump is applied ON the pull request branch, not on the default + # branch and moved across afterwards. Bumping first and stashing the + # result over a checkout collides whenever the branch already carries + # a bump for the same chart, and a swallowed stash conflict either + # drops that earlier bump or commits conflict markers. Starting here + # also makes the run idempotent: the bumper sees the current value and + # reports "already ". + git config user.name "github-actions[bot]" + git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + git fetch origin "${BRANCH}" || true + if git rev-parse --verify -q "origin/${BRANCH}" >/dev/null; then + git checkout -B "${BRANCH}" "origin/${BRANCH}" + else + git checkout -B "${BRANCH}" + fi + - name: Apply the bump id: bump if: steps.tag.outputs.applies == 'true' + env: + # Through env, never expanded into the script body: a tag is chosen by + # whoever pushes it, and ${{ }} interpolation into a run: block is the + # standard Actions injection shape. + TAG: ${{ steps.tag.outputs.tag }} run: | set +e set -uo pipefail @@ -100,7 +127,7 @@ jobs: # it still applied every chart it could move safely. Aborting here # would throw those away and leave the refusal as the only outcome. tools/ci/chart-version-bumper \ - --tag "${{ steps.tag.outputs.tag }}" --write 2>/tmp/refusals + --tag "${TAG}" --write 2>/tmp/refusals status=$? cat /tmp/refusals >&2 # The exit code, not whether stderr is empty. SystemExit writes its @@ -135,23 +162,11 @@ jobs: GH_TOKEN: ${{ secrets.NV_GITHUB_TOKEN || github.token }} TAG: ${{ steps.tag.outputs.tag }} REFUSED: ${{ steps.bump.outputs.refused }} + SERVER_URL: ${{ github.server_url }} + REPO: ${{ github.repository }} run: | set -euo pipefail branch="chore/chart-version-bumps" - git config user.name "nvcf-release-bot" - git config user.email "svc-nvcf-release@nvidia.com" - - # A fixed branch, refreshed. Several releases landing close together - # then produce one pull request carrying all of their bumps instead - # of a pile that conflict with each other. - git fetch origin "${branch}" || true - if git rev-parse --verify -q "origin/${branch}" >/dev/null; then - git stash push --quiet - git checkout -B "${branch}" "origin/${branch}" - git stash pop --quiet || true - else - git checkout -B "${branch}" - fi git add deploy/helm # Separate -m flags rather than an embedded multi-line string: the @@ -159,8 +174,7 @@ jobs: # ends the YAML block scalar this script lives in. git commit \ -m "chore(charts): bump for ${TAG}" \ - -m "Opened by the chart version bump workflow on release of ${TAG}." \ - -m "Co-authored-by: Balaji Ganesan " + -m "Opened by the chart version bump workflow on release of ${TAG}." git push --force-with-lease origin "${branch}" notes="" @@ -183,19 +197,24 @@ jobs: "" \ "Merging this does not move the self-managed stack. A chart version reaches the stack only once the chart itself is released, and publishing that chart release is what triggers \`stack-pin-bump.yml\`." \ "" \ - "Release notes: ${{ github.server_url }}/${{ github.repository }}/releases/tag/${TAG}" \ + "Release notes: ${SERVER_URL}/${REPO}/releases/tag/${TAG}" \ "${notes}" \ "" \ "If this pull request sits unmerged, later service releases add their bumps to the same branch, so merging it applies all of them." \ "" \ "Github commit:" \ "chore(charts): bump chart versions for released services" \ - "" \ - "Co-authored-by: Balaji Ganesan ")" - - if gh pr view "${branch}" --json number >/dev/null 2>&1; then - gh pr edit "${branch}" --body "${body}" - echo "refreshed the existing pull request" + "")" + + # gh api, not `gh pr edit`. Against this repository `gh pr edit` fails + # with "Projects (classic) is being deprecated ... + # (repository.pullRequest.projectCards)", because it queries project + # cards it does not need. The REST endpoint has no such dependency. + number="$(gh api "repos/${REPO}/pulls?head=${REPO%%/*}:${branch}&state=open" -q '.[0].number')" + if [ -n "${number}" ] && [ "${number}" != "null" ]; then + jq -n --arg b "${body}" '{body: $b}' \ + | gh api -X PATCH "repos/${REPO}/pulls/${number}" --input - >/dev/null + echo "refreshed pull request #${number}" else gh pr create --base main --head "${branch}" \ --title "chore(charts): bump chart versions for released services" \ From aac0d9828ada8eccab138ccc71feaf8e5328327a Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Wed, 26 Aug 2026 12:48:34 -0700 Subject: [PATCH 4/9] fix(ci): make the generated chart bump commit cut a chart release The cascade stopped after its first hop. tools/ci/github-release feeds RELEASE_RULES to semantic-release, where chore carries "release": false, and release-tags.yml runs `github-release auto` on every push to main with NVCF_GITHUB_AUTO_TAGGING_ENABLED=true and NVCF_GITHUB_RELEASE_DRY_RUN=false. So a generated commit of `chore(charts): bump for ` cuts no chart release. Publishing a chart release is exactly what triggers stack-pin-bump.yml, so the chart would carry the new appVersion on main while the stack never learned about it, and the run would look successful throughout. The generated commit and pull request title are now `fix(charts):`, which cuts a patch release of the chart and lets the second hop fire. A patch is the right size. The chart's own templates and values schema have not changed, only the application version it defaults to, which is the conventional reading of chart version against appVersion. Co-authored-by: Balaji Ganesan Signed-off-by: Balaji Ganesan --- .github/workflows/chart-version-bump.yml | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/.github/workflows/chart-version-bump.yml b/.github/workflows/chart-version-bump.yml index 985132c08..10da4d009 100644 --- a/.github/workflows/chart-version-bump.yml +++ b/.github/workflows/chart-version-bump.yml @@ -172,8 +172,19 @@ jobs: # Separate -m flags rather than an embedded multi-line string: the # continuation lines of one would have to sit at column zero, which # ends the YAML block scalar this script lives in. + # fix, not chore. tools/ci/github-release feeds RELEASE_RULES to + # semantic-release, where chore carries "release": false. A chore + # commit therefore cuts no chart release, and since publishing a chart + # release is exactly what triggers stack-pin-bump.yml, the cascade + # would stop here: the chart would carry the new appVersion on main + # and the stack would never learn about it. + # + # A patch bump of the chart is the right size. The chart's own + # templates and values schema have not changed, only the application + # version it defaults to, which is the conventional reading of chart + # version against appVersion. git commit \ - -m "chore(charts): bump for ${TAG}" \ + -m "fix(charts): bump for ${TAG}" \ -m "Opened by the chart version bump workflow on release of ${TAG}." git push --force-with-lease origin "${branch}" @@ -203,7 +214,7 @@ jobs: "If this pull request sits unmerged, later service releases add their bumps to the same branch, so merging it applies all of them." \ "" \ "Github commit:" \ - "chore(charts): bump chart versions for released services" \ + "fix(charts): bump chart versions for released services" \ "")" # gh api, not `gh pr edit`. Against this repository `gh pr edit` fails @@ -217,7 +228,7 @@ jobs: echo "refreshed pull request #${number}" else gh pr create --base main --head "${branch}" \ - --title "chore(charts): bump chart versions for released services" \ + --title "fix(charts): bump chart versions for released services" \ --body "${body}" fi From 4f20454a337d1090c4e350a2aa563365c97f9f62 Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Wed, 26 Aug 2026 16:32:12 -0700 Subject: [PATCH 5/9] fix(ci): stop a floating tag on another image refusing the chart Three review findings. A chart may ship several images, and only the tag equal to appVersion is rewritten, so a sidecar pinned to latest is none of the released service's business. Checking floating before agreement refused the whole chart over an image it never touches, and the refusal turned the run red and blocked a bump that was entirely safe. Agreement is now checked first; floating still refuses when no tag agrees, because then one of those tags is this service's image and none of them is rewritable. Apply wrote Chart.yaml and then read values.yaml. A failed read left appVersion moved with the image tag behind, which is exactly the drift state the next run refuses: one partial write would poison the chart for every future bump. Both files are read before the first write. The release tag reached the "Select the tag" step through ${{ }} interpolation rather than env, unlike every other step here. A tag is chosen by whoever pushes it, so that is the standard Actions injection shape. Three tests, each mutation checked against a precise inversion of its fix: reordering the floating check ahead of agreement kills only the new floating-sidecar test, and reading values.yaml after the Chart.yaml write kills only the new partial-write test. The step body was exercised against five tag shapes including one carrying a shell metacharacter, which is inert. Declined: errcheck on the report-writing fmt.Fprintf calls. No golangci config or errcheck run covers tools/, and a stdout write failure in a CLI has nowhere to be reported; CI acts on the exit code. Co-authored-by: Balaji Ganesan Signed-off-by: Balaji Ganesan --- .github/workflows/chart-version-bump.yml | 8 ++- tools/chart-version-bumper/chart.go | 38 ++++++++----- tools/chart-version-bumper/main_test.go | 69 ++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 13 deletions(-) diff --git a/.github/workflows/chart-version-bump.yml b/.github/workflows/chart-version-bump.yml index 10da4d009..ae4a880f5 100644 --- a/.github/workflows/chart-version-bump.yml +++ b/.github/workflows/chart-version-bump.yml @@ -67,9 +67,15 @@ jobs: - name: Select the tag id: tag + env: + # Through env like every other step here. A release tag is chosen by + # whoever pushes it, so expanding it into the script body is the + # standard Actions injection shape. + INPUT_TAG: ${{ github.event.inputs.tag }} + RELEASE_TAG: ${{ github.event.release.tag_name }} run: | set -euo pipefail - tag="${{ github.event.inputs.tag || github.event.release.tag_name }}" + tag="${INPUT_TAG:-${RELEASE_TAG}}" echo "tag=${tag}" >> "${GITHUB_OUTPUT}" # Chart releases move stack pins, which is stack-pin-bump.yml's job. # Everything else is a service release and is this job's business: diff --git a/tools/chart-version-bumper/chart.go b/tools/chart-version-bumper/chart.go index 68d92dbae..47ae3acae 100644 --- a/tools/chart-version-bumper/chart.go +++ b/tools/chart-version-bumper/chart.go @@ -97,23 +97,30 @@ func PlanFor(root string, chart Entry, version string) (Plan, error) { return Plan{}, fmt.Errorf("read %s: %w", valuesYAML, err) } - var floats []string + // Agreement first. A chart may ship several images, and only the tag equal + // to appVersion is rewritten, so a floating tag on a different image is none + // of this service's business. Checking floating first refused the whole + // chart because a sidecar was pinned to latest. for _, t := range tags { - if floating[t] { - floats = append(floats, t) + if t == current { + return Plan{ActionBoth, "appVersion and image tag agree", current, tags}, nil } } - if len(floats) > 0 { - return Plan{ActionRefuse, "image tag is floating (" + strings.Join(floats, ", ") + ")", current, tags}, nil - } if len(tags) == 0 { return Plan{ActionAppVersionOnly, "no image tag set", current, tags}, nil } + // No tag agrees, so one of these is this service's image. If any of them + // floats there is nothing safe to rewrite: latest is not a pin, and + // replacing it with a version is a behaviour change rather than a bump. + var floats []string for _, t := range tags { - if t == current { - return Plan{ActionBoth, "appVersion and image tag agree", current, tags}, nil + if floating[t] { + floats = append(floats, t) } } + if len(floats) > 0 { + return Plan{ActionRefuse, "image tag is floating (" + strings.Join(floats, ", ") + ")", current, tags}, nil + } return Plan{ ActionRefuse, fmt.Sprintf("appVersion %s does not match image tag(s) %s", current, strings.Join(tags, ", ")), @@ -132,6 +139,17 @@ func Apply(root string, chart Entry, version string, p Plan) error { text := string(b) current := appVersionRE.FindStringSubmatch(text)[2] + // Read before the first write. Writing Chart.yaml and then failing to read + // values.yaml leaves appVersion moved with the image tag behind, which is + // exactly the drift state the next run refuses. + var vb []byte + if p.Action == ActionBoth { + vb, err = os.ReadFile(valuesYAML) + if err != nil { + return fmt.Errorf("read %s: %w", valuesYAML, err) + } + } + replaced := false updated := appVersionRE.ReplaceAllStringFunc(text, func(line string) string { if replaced { @@ -151,10 +169,6 @@ func Apply(root string, chart Entry, version string, p Plan) error { // Replace only tag lines holding the value appVersion also held. Any other // tag in this file belongs to a different image, and moving it would point // a sidecar at a version that was never built for it. - vb, err := os.ReadFile(valuesYAML) - if err != nil { - return fmt.Errorf("read %s: %w", valuesYAML, err) - } matching := regexp.MustCompile(`(?m)^(\s+tag:\s*)"?` + regexp.QuoteMeta(current) + `"?(\s*(?:#.*)?)$`) out := matching.ReplaceAllString(string(vb), fmt.Sprintf(`${1}"%s"${2}`, version)) return writeFilePreservingMode(valuesYAML, out) diff --git a/tools/chart-version-bumper/main_test.go b/tools/chart-version-bumper/main_test.go index 2d0b6186d..7653c4e76 100644 --- a/tools/chart-version-bumper/main_test.go +++ b/tools/chart-version-bumper/main_test.go @@ -333,3 +333,72 @@ func repoRoot(t *testing.T) string { t.Fatalf("could not find %s above the test directory", MetadataPath) return "" } + +func TestFloatingTagOnAnotherImageDoesNotRefuseTheChart(t *testing.T) { + // Only the tag equal to appVersion is rewritten, so a sidecar pinned to + // latest is none of this service's business. Checking floating before + // agreement refused the whole chart because of an image it never touches. + f := newFixture(t, `{"services":[ + {"id":"svc","path":"src/svc"}, + {"id":"mixed","path":"deploy/helm/mixed","deploys":["svc"]} + ]}`) + f.chart(t, "mixed", "1.0.0", "app:\n image:\n tag: \"1.0.0\"\nsidecar:\n image:\n tag: \"latest\"") + + code, out, errOut := f.run(t, "src/svc/v2.0.0", true) + if code != 0 { + t.Fatalf("want a clean bump, got exit %d\n%s%s", code, out, errOut) + } + values := f.read(t, "mixed", "values.yaml") + if !strings.Contains(values, ` tag: "2.0.0"`) { + t.Fatalf("the matching tag should have moved:\n%s", values) + } + if !strings.Contains(values, ` tag: "latest"`) { + t.Fatalf("the floating sidecar tag must be left alone:\n%s", values) + } +} + +func TestFloatingStillRefusesWhenNoTagAgrees(t *testing.T) { + // The other side of the reorder: if nothing agrees with appVersion, one of + // these tags is this service's image, and a floating one is not rewritable. + f := newFixture(t, `{"services":[ + {"id":"svc","path":"src/svc"}, + {"id":"floater","path":"deploy/helm/floater","deploys":["svc"]} + ]}`) + f.chart(t, "floater", "1.0.0", "image:\n tag: \"latest\"") + code, _, errOut := f.run(t, "src/svc/v2.0.0", true) + if code != RefusedExit { + t.Fatalf("want refusal, got %d", code) + } + if !strings.Contains(errOut, "image tag is floating") { + t.Fatalf("want the floating reason:\n%s", errOut) + } + if got := f.read(t, "floater", "values.yaml"); !strings.Contains(got, `tag: "latest"`) { + t.Fatalf("nothing may be written:\n%s", got) + } +} + +func TestAMissingValuesFileLeavesChartYamlAlone(t *testing.T) { + // Apply reads values.yaml before writing Chart.yaml. Writing first and then + // failing the read leaves appVersion moved with the tag behind, which is the + // drift state the next run refuses: a partial write that poisons the chart. + f := newFixture(t, `{"services":[ + {"id":"svc","path":"src/svc"}, + {"id":"c","path":"deploy/helm/c","deploys":["svc"]} + ]}`) + f.chart(t, "c", "1.0.0", "image:\n tag: \"1.0.0\"") + before := f.read(t, "c", "Chart.yaml") + if err := os.Remove(filepath.Join(f.root, "deploy", "helm", "c", "values.yaml")); err != nil { + t.Fatal(err) + } + + // With values.yaml gone the chart plans appversion-only, so force the + // both-file path directly to exercise the ordering. + chart := Entry{ID: "c", Path: "deploy/helm/c", Deploys: []string{"svc"}} + err := Apply(f.root, chart, "2.0.0", Plan{Action: ActionBoth, Current: "1.0.0"}) + if err == nil { + t.Fatal("Apply should fail when values.yaml cannot be read") + } + if after := f.read(t, "c", "Chart.yaml"); after != before { + t.Fatalf("Chart.yaml was written despite the failure:\nbefore:\n%s\nafter:\n%s", before, after) + } +} From a5095d087a90a5dbec9c70f64a3b6a02bb538f83 Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Thu, 27 Aug 2026 19:07:31 -0700 Subject: [PATCH 6/9] fix(ci): refuse a chart whose service image cannot be identified A tag equal to appVersion was treated as evidence that it is the released service's image. It is not. A chart whose appVersion has fallen behind its own image can still match an unrelated sidecar that happens to sit on that version. Service image at 2.0.0, sidecar at 1.0.0, appVersion 1.0.0: the bump moved appVersion and the sidecar to the released version and left the service image untouched. A wrong edit that reads as a routine version bump is exactly the failure this tool exists to avoid. Agreement is only unambiguous when the chart ships one image. With more than one and nothing marking which is the service's, the chart is refused and the reason says the tag to move cannot be identified. Nothing is blocked by this today: every chart with a declared service edge ships zero or one image tag. A chart that grows a second image needs a way to name its service tag before it can be bumped, which is a smaller and better-informed change than guessing now. Apply keeps its line-scoped rewrite and stays tested directly, since that is what would protect a sidecar once such a selector exists. Three tests, mutation checked: bumping a multi-image chart anyway, dropping the single-tag agreement requirement, and accepting a floating tag are each caught. The multi-image fixture is the reported case, service 2.0.0, sidecar 1.0.0, appVersion 1.0.0 and a third image on latest, and asserts that neither values.yaml nor Chart.yaml is written. Co-authored-by: Balaji Ganesan Signed-off-by: Balaji Ganesan --- tools/chart-version-bumper/chart.go | 50 +++++++++------- tools/chart-version-bumper/main_test.go | 78 ++++++++++++++++--------- 2 files changed, 82 insertions(+), 46 deletions(-) diff --git a/tools/chart-version-bumper/chart.go b/tools/chart-version-bumper/chart.go index 47ae3acae..fc8506502 100644 --- a/tools/chart-version-bumper/chart.go +++ b/tools/chart-version-bumper/chart.go @@ -97,29 +97,39 @@ func PlanFor(root string, chart Entry, version string) (Plan, error) { return Plan{}, fmt.Errorf("read %s: %w", valuesYAML, err) } - // Agreement first. A chart may ship several images, and only the tag equal - // to appVersion is rewritten, so a floating tag on a different image is none - // of this service's business. Checking floating first refused the whole - // chart because a sidecar was pinned to latest. - for _, t := range tags { - if t == current { - return Plan{ActionBoth, "appVersion and image tag agree", current, tags}, nil - } - } if len(tags) == 0 { return Plan{ActionAppVersionOnly, "no image tag set", current, tags}, nil } - // No tag agrees, so one of these is this service's image. If any of them - // floats there is nothing safe to rewrite: latest is not a pin, and - // replacing it with a version is a behaviour change rather than a bump. - var floats []string - for _, t := range tags { - if floating[t] { - floats = append(floats, t) - } - } - if len(floats) > 0 { - return Plan{ActionRefuse, "image tag is floating (" + strings.Join(floats, ", ") + ")", current, tags}, nil + + // More than one image, and nothing says which belongs to the released + // service. A tag equal to appVersion is not evidence of ownership: a chart + // whose appVersion has fallen behind its own image can still match an + // unrelated sidecar that happens to sit on that version, and the bump would + // then move the sidecar and leave the service image alone. That is a wrong + // edit dressed as a routine version bump, which is the failure this tool + // exists to avoid, so it refuses instead. + // + // Every chart with a declared service edge currently ships zero or one image + // tag, so nothing is blocked by this today. Charts that grow a second image + // need a way to name the service's own tag before they can be bumped. + if len(tags) > 1 { + return Plan{ + ActionRefuse, + fmt.Sprintf("chart declares %d image tags (%s) and none is marked as this service's, so the one to move cannot be identified", + len(tags), strings.Join(tags, ", ")), + current, + tags, + }, nil + } + + // Exactly one image, so agreement is unambiguous evidence. + if floating[tags[0]] { + // latest is not a pin, and replacing it with a version is a behaviour + // change rather than a bump. + return Plan{ActionRefuse, "image tag is floating (" + tags[0] + ")", current, tags}, nil + } + if tags[0] == current { + return Plan{ActionBoth, "appVersion and image tag agree", current, tags}, nil } return Plan{ ActionRefuse, diff --git a/tools/chart-version-bumper/main_test.go b/tools/chart-version-bumper/main_test.go index 7653c4e76..ca1a8072b 100644 --- a/tools/chart-version-bumper/main_test.go +++ b/tools/chart-version-bumper/main_test.go @@ -208,15 +208,19 @@ func TestChartReleaseTagIsNotAServiceRelease(t *testing.T) { } } -func TestWriteMovesAppVersionAndOnlyTheMatchingTag(t *testing.T) { +func TestApplyRewritesOnlyTheTagLinesMatchingAppVersion(t *testing.T) { + // Apply is exercised directly. PlanFor refuses a multi-image chart now, but + // the line-scoped rewrite is still the mechanism that protects a sidecar if + // a chart ever gains a way to name its service image, so it stays covered. f := newFixture(t, `{"services":[ {"id":"svc","path":"src/svc"}, {"id":"multi","path":"deploy/helm/multi","deploys":["svc"]} ]}`) f.chart(t, "multi", "1.0.0", "app:\n image:\n tag: \"1.0.0\"\nsidecar:\n image:\n tag: \"3.3.3\"") - if code, _, errOut := f.run(t, "src/svc/v2.0.0", true); code != 0 { - t.Fatalf("write should succeed, got %d: %s", code, errOut) + chart := Entry{ID: "multi", Path: "deploy/helm/multi", Deploys: []string{"svc"}} + if err := Apply(f.root, chart, "2.0.0", Plan{Action: ActionBoth, Current: "1.0.0"}); err != nil { + t.Fatalf("Apply: %v", err) } if got := f.read(t, "multi", "Chart.yaml"); !strings.Contains(got, `appVersion: "2.0.0"`) { t.Fatalf("appVersion did not move:\n%s", got) @@ -230,6 +234,51 @@ func TestWriteMovesAppVersionAndOnlyTheMatchingTag(t *testing.T) { } } +func TestMultipleImagesRefuseBecauseOwnershipIsUnknown(t *testing.T) { + // A tag equal to appVersion is not evidence that it is the service's image. + // This is the case that makes it not evidence: appVersion has fallen behind + // the service image at 2.0.0 and instead matches an unrelated sidecar at + // 1.0.0. Treating agreement as ownership moves the sidecar to the released + // version and leaves the service image untouched, which is a wrong edit that + // reads as a routine bump. + f := newFixture(t, `{"services":[ + {"id":"svc","path":"src/svc"}, + {"id":"drifted","path":"deploy/helm/drifted","deploys":["svc"]} + ]}`) + f.chart(t, "drifted", "1.0.0", + "service:\n image:\n tag: \"2.0.0\"\nsidecar:\n image:\n tag: \"1.0.0\"\nother:\n image:\n tag: \"latest\"") + before := f.read(t, "drifted", "values.yaml") + + code, _, errOut := f.run(t, "src/svc/v3.0.0", true) + if code != RefusedExit { + t.Fatalf("want refusal, got exit %d", code) + } + if !strings.Contains(errOut, "cannot be identified") { + t.Fatalf("the refusal should say ownership is unknown:\n%s", errOut) + } + if after := f.read(t, "drifted", "values.yaml"); after != before { + t.Fatalf("nothing may be written:\n%s", after) + } + if got := f.read(t, "drifted", "Chart.yaml"); !strings.Contains(got, `appVersion: "1.0.0"`) { + t.Fatalf("appVersion must not move either:\n%s", got) + } +} + +func TestSingleFloatingTagStillRefuses(t *testing.T) { + f := newFixture(t, `{"services":[ + {"id":"svc","path":"src/svc"}, + {"id":"floater","path":"deploy/helm/floater","deploys":["svc"]} + ]}`) + f.chart(t, "floater", "1.0.0", "image:\n tag: \"latest\"") + code, _, errOut := f.run(t, "src/svc/v2.0.0", true) + if code != RefusedExit { + t.Fatalf("want refusal, got %d", code) + } + if !strings.Contains(errOut, "image tag is floating") { + t.Fatalf("want the floating reason:\n%s", errOut) + } +} + func TestWritePreservesSurroundingContent(t *testing.T) { // Rewriting line by line rather than round-tripping YAML is deliberate; this // is the test that fails if someone swaps in a marshaller. @@ -334,29 +383,6 @@ func repoRoot(t *testing.T) string { return "" } -func TestFloatingTagOnAnotherImageDoesNotRefuseTheChart(t *testing.T) { - // Only the tag equal to appVersion is rewritten, so a sidecar pinned to - // latest is none of this service's business. Checking floating before - // agreement refused the whole chart because of an image it never touches. - f := newFixture(t, `{"services":[ - {"id":"svc","path":"src/svc"}, - {"id":"mixed","path":"deploy/helm/mixed","deploys":["svc"]} - ]}`) - f.chart(t, "mixed", "1.0.0", "app:\n image:\n tag: \"1.0.0\"\nsidecar:\n image:\n tag: \"latest\"") - - code, out, errOut := f.run(t, "src/svc/v2.0.0", true) - if code != 0 { - t.Fatalf("want a clean bump, got exit %d\n%s%s", code, out, errOut) - } - values := f.read(t, "mixed", "values.yaml") - if !strings.Contains(values, ` tag: "2.0.0"`) { - t.Fatalf("the matching tag should have moved:\n%s", values) - } - if !strings.Contains(values, ` tag: "latest"`) { - t.Fatalf("the floating sidecar tag must be left alone:\n%s", values) - } -} - func TestFloatingStillRefusesWhenNoTagAgrees(t *testing.T) { // The other side of the reorder: if nothing agrees with appVersion, one of // these tags is this service's image, and a floating one is not rewritable. From e1f00e8ad022e03db80377ba6a06b1062e7c2dd2 Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Thu, 27 Aug 2026 19:50:31 -0700 Subject: [PATCH 7/9] fix(ci): identify image tags by structure, not by key name The tag scan matched every indented tag: key in values.yaml, so a key named tag: outside an image block counted as an image tag. One holding the same string as appVersion would be selected and rewritten even though it has nothing to do with an image, and one holding something else looked like a second image and refused a chart whose only real image tag was fine. Ownership is now decided by where the key sits: a tag: is an image tag only when its nearest enclosing key is image. Apply rewrites those exact lines rather than running a regex over the whole file, so a matching value elsewhere is not reachable either. No chart changes behaviour. All thirteen tag: keys across the charts with a declared service edge already sit directly under image:, which is what makes the structural rule free to adopt now rather than after something breaks. Three tests, mutation checked: counting any tag: as an image tag, ignoring indent when finding the parent key, and rewriting every image tag rather than only the matching one are each caught. The fixtures cover a non-image tag that would be falsely selected, one that would cause a false refusal, and an image block nested under a component key. Co-authored-by: Balaji Ganesan Signed-off-by: Balaji Ganesan --- tools/chart-version-bumper/chart.go | 62 +++++++++++++++++++++---- tools/chart-version-bumper/main_test.go | 58 +++++++++++++++++++++++ 2 files changed, 112 insertions(+), 8 deletions(-) diff --git a/tools/chart-version-bumper/chart.go b/tools/chart-version-bumper/chart.go index fc8506502..15b7d7549 100644 --- a/tools/chart-version-bumper/chart.go +++ b/tools/chart-version-bumper/chart.go @@ -40,9 +40,47 @@ var floating = map[string]bool{ // style across the whole file for the sake of one value. var ( appVersionRE = regexp.MustCompile(`(?m)^(appVersion:\s*)"?([^"\s#]+)"?(.*)$`) - tagRE = regexp.MustCompile(`(?m)^\s+tag:\s*"?([^"\s#]*)"?`) + tagLineRE = regexp.MustCompile(`^(\s+)tag:\s*"?([^"\s#]*)"?`) + keyLineRE = regexp.MustCompile(`^(\s*)([A-Za-z0-9_.-]+):`) ) +// An imageTag is a tag: entry that sits directly under an image: key, together +// with the line it was found on. +type imageTag struct { + line int + value string +} + +// imageTags returns the tag: entries that belong to an image block. +// +// Matching every indented tag: key instead would reach unrelated fields. A +// values.yaml may carry a tag: that is not an image tag at all, and one of those +// holding the same string as appVersion would be selected and rewritten, while +// one holding something else could refuse a chart whose image tag was fine. +// Ownership is decided by where the key sits, not by what it is called. +func imageTags(lines []string) []imageTag { + var out []imageTag + for i, l := range lines { + m := tagLineRE.FindStringSubmatch(l) + if m == nil || m[2] == "" { + continue + } + indent := len(m[1]) + // The nearest preceding key at a smaller indent is this key's parent. + for j := i - 1; j >= 0; j-- { + pm := keyLineRE.FindStringSubmatch(lines[j]) + if pm == nil || len(pm[1]) >= indent { + continue + } + if pm[2] == "image" { + out = append(out, imageTag{line: i, value: m[2]}) + } + break + } + } + return out +} + // A Plan is what to do for one chart. type Plan struct { Action Action @@ -88,10 +126,8 @@ func PlanFor(root string, chart Entry, version string) (Plan, error) { var tags []string if vb, err := os.ReadFile(valuesYAML); err == nil { - for _, hit := range tagRE.FindAllStringSubmatch(string(vb), -1) { - if hit[1] != "" { - tags = append(tags, hit[1]) - } + for _, it := range imageTags(strings.Split(string(vb), "\n")) { + tags = append(tags, it.value) } } else if !os.IsNotExist(err) { return Plan{}, fmt.Errorf("read %s: %w", valuesYAML, err) @@ -179,9 +215,19 @@ func Apply(root string, chart Entry, version string, p Plan) error { // Replace only tag lines holding the value appVersion also held. Any other // tag in this file belongs to a different image, and moving it would point // a sidecar at a version that was never built for it. - matching := regexp.MustCompile(`(?m)^(\s+tag:\s*)"?` + regexp.QuoteMeta(current) + `"?(\s*(?:#.*)?)$`) - out := matching.ReplaceAllString(string(vb), fmt.Sprintf(`${1}"%s"${2}`, version)) - return writeFilePreservingMode(valuesYAML, out) + // Rewrite by line, and only lines imageTags identified. A regex over the + // whole file would reach a tag: outside an image block that happens to hold + // the same value. + lines := strings.Split(string(vb), "\n") + for _, it := range imageTags(lines) { + if it.value != current { + continue + } + m := tagLineRE.FindStringSubmatch(lines[it.line]) + suffix := lines[it.line][len(m[0]):] + lines[it.line] = fmt.Sprintf("%stag: %q%s", m[1], version, suffix) + } + return writeFilePreservingMode(valuesYAML, strings.Join(lines, "\n")) } func writeFilePreservingMode(path, content string) error { diff --git a/tools/chart-version-bumper/main_test.go b/tools/chart-version-bumper/main_test.go index ca1a8072b..1121ffced 100644 --- a/tools/chart-version-bumper/main_test.go +++ b/tools/chart-version-bumper/main_test.go @@ -428,3 +428,61 @@ func TestAMissingValuesFileLeavesChartYamlAlone(t *testing.T) { t.Fatalf("Chart.yaml was written despite the failure:\nbefore:\n%s\nafter:\n%s", before, after) } } + +func TestNonImageTagIsNotTreatedAsAnImageTag(t *testing.T) { + // A tag: outside an image block is not an image tag. Matching every + // indented tag: key would select this one, because its value equals + // appVersion, and rewrite a field that has nothing to do with the image. + f := newFixture(t, `{"services":[ + {"id":"svc","path":"src/svc"}, + {"id":"c","path":"deploy/helm/c","deploys":["svc"]} + ]}`) + f.chart(t, "c", "1.0.0", + "release:\n tag: \"1.0.0\"\nimage:\n tag: \"1.0.0\"") + + if code, out, errOut := f.run(t, "src/svc/v2.0.0", true); code != 0 { + t.Fatalf("one image tag means an unambiguous bump, got %d\n%s%s", code, out, errOut) + } + values := f.read(t, "c", "values.yaml") + if !strings.Contains(values, "release:\n tag: \"1.0.0\"") { + t.Fatalf("the non-image tag must be left alone:\n%s", values) + } + if !strings.Contains(values, "image:\n tag: \"2.0.0\"") { + t.Fatalf("the image tag should have moved:\n%s", values) + } +} + +func TestNonImageTagDoesNotCauseAFalseRefusal(t *testing.T) { + // The other direction. A non-image tag holding something else would look + // like a second image, and the chart would be refused as ambiguous even + // though its only real image tag agrees with appVersion. + f := newFixture(t, `{"services":[ + {"id":"svc","path":"src/svc"}, + {"id":"c","path":"deploy/helm/c","deploys":["svc"]} + ]}`) + f.chart(t, "c", "1.0.0", + "metadata:\n tag: \"someLabel\"\nimage:\n tag: \"1.0.0\"") + + code, out, errOut := f.run(t, "src/svc/v2.0.0", false) + if code != 0 { + t.Fatalf("want a clean plan, got exit %d\n%s%s", code, out, errOut) + } + if !strings.Contains(out, "appVersion and image tag agree") { + t.Fatalf("want the agreement plan:\n%s", out) + } +} + +func TestImageTagNestedDeeperIsStillFound(t *testing.T) { + // The image block is often nested under a component key. + f := newFixture(t, `{"services":[ + {"id":"svc","path":"src/svc"}, + {"id":"c","path":"deploy/helm/c","deploys":["svc"]} + ]}`) + f.chart(t, "c", "1.0.0", "app:\n image:\n repository: \"\"\n tag: \"1.0.0\"") + if code, out, errOut := f.run(t, "src/svc/v2.0.0", true); code != 0 { + t.Fatalf("want a clean bump, got %d\n%s%s", code, out, errOut) + } + if got := f.read(t, "c", "values.yaml"); !strings.Contains(got, ` tag: "2.0.0"`) { + t.Fatalf("nested image tag did not move:\n%s", got) + } +} From 3d43ca978d6dfe118c1c9fe38d40cd05e5423772 Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Thu, 27 Aug 2026 20:07:00 -0700 Subject: [PATCH 8/9] fix(ci): refuse an inline image declaration instead of half-bumping A chart may declare its image on one line, either as a flow mapping, image: { tag: "1.0.0" }, or as a plain reference, image: registry/name:tag. The line scan finds no tag in either, which is indistinguishable from a chart that sets no tag at all, so appVersion would move on its own and the deployed image would stay where it was: a chart that looks bumped and is not. Refused rather than parsed. A YAML parser is a large answer to a shape no chart with a declared service edge uses, and a stop is recoverable where a silent half-bump is not. deploy/helm/nvcf-unbound and deploy/helm/openbao already declare an image this way, so the shape is real even though neither is reachable from a service release today. Five tests, mutation checked: removing the refusal is caught. They cover a flow mapping, a plain image reference, a flow mapping nested under a component, and the ordinary block form, which must still bump. Co-authored-by: Balaji Ganesan Signed-off-by: Balaji Ganesan --- tools/chart-version-bumper/chart.go | 20 ++++++++++- tools/chart-version-bumper/main_test.go | 44 +++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/tools/chart-version-bumper/chart.go b/tools/chart-version-bumper/chart.go index 15b7d7549..4e0f8fb56 100644 --- a/tools/chart-version-bumper/chart.go +++ b/tools/chart-version-bumper/chart.go @@ -41,7 +41,11 @@ var floating = map[string]bool{ var ( appVersionRE = regexp.MustCompile(`(?m)^(appVersion:\s*)"?([^"\s#]+)"?(.*)$`) tagLineRE = regexp.MustCompile(`^(\s+)tag:\s*"?([^"\s#]*)"?`) - keyLineRE = regexp.MustCompile(`^(\s*)([A-Za-z0-9_.-]+):`) + // An image: key carrying a value on the same line, rather than opening a + // block. Covers a flow mapping, image: { tag: "1.0.0" }, and a plain scalar + // reference, image: registry/name:tag. + inlineImageRE = regexp.MustCompile(`(?m)^\s*image:[ \t]*[^ \t\n#].*$`) + keyLineRE = regexp.MustCompile(`^(\s*)([A-Za-z0-9_.-]+):`) ) // An imageTag is a tag: entry that sits directly under an image: key, together @@ -126,6 +130,20 @@ func PlanFor(root string, chart Entry, version string) (Plan, error) { var tags []string if vb, err := os.ReadFile(valuesYAML); err == nil { + // An image declared inline rather than as a block is refused, not parsed. + // imageTags finds nothing in it, which would look identical to a chart + // that sets no tag at all: appVersion would move on its own and the + // deployed image would stay where it was. A silent half-bump is worse + // than a stop, and a YAML parser is a large answer to a shape no chart + // here uses. + if m := inlineImageRE.FindString(string(vb)); m != "" { + return Plan{ + ActionRefuse, + fmt.Sprintf("image is declared inline (%s), so its tag cannot be located", strings.TrimSpace(m)), + current, + nil, + }, nil + } for _, it := range imageTags(strings.Split(string(vb), "\n")) { tags = append(tags, it.value) } diff --git a/tools/chart-version-bumper/main_test.go b/tools/chart-version-bumper/main_test.go index 1121ffced..90613fc60 100644 --- a/tools/chart-version-bumper/main_test.go +++ b/tools/chart-version-bumper/main_test.go @@ -486,3 +486,47 @@ func TestImageTagNestedDeeperIsStillFound(t *testing.T) { t.Fatalf("nested image tag did not move:\n%s", got) } } + +func TestInlineImageMappingRefusesRatherThanHalfBumping(t *testing.T) { + // `image: { tag: "1.0.0" }` is valid YAML that the line scan cannot see. Read + // as "no image tag set", appVersion would move on its own and the deployed + // image would stay where it was: a chart that looks bumped and is not. + for _, values := range []string{ + `image: { tag: "1.0.0" }`, + `image: registry.example/app:1.0.0`, + "app:\n image: { repository: \"r\", tag: \"1.0.0\" }", + } { + f := newFixture(t, `{"services":[ + {"id":"svc","path":"src/svc"}, + {"id":"c","path":"deploy/helm/c","deploys":["svc"]} + ]}`) + f.chart(t, "c", "1.0.0", values) + before := f.read(t, "c", "Chart.yaml") + + code, _, errOut := f.run(t, "src/svc/v2.0.0", true) + if code != RefusedExit { + t.Errorf("values %q: want refusal, got exit %d", values, code) + } + if !strings.Contains(errOut, "declared inline") { + t.Errorf("values %q: want the inline reason, got %q", values, errOut) + } + if after := f.read(t, "c", "Chart.yaml"); after != before { + t.Errorf("values %q: appVersion moved despite the refusal:\n%s", values, after) + } + } +} + +func TestBlockImageIsStillAccepted(t *testing.T) { + // The refusal must not catch the ordinary block form, which every chart uses. + f := newFixture(t, `{"services":[ + {"id":"svc","path":"src/svc"}, + {"id":"c","path":"deploy/helm/c","deploys":["svc"]} + ]}`) + f.chart(t, "c", "1.0.0", "image:\n repository: \"\"\n tag: \"1.0.0\"") + if code, out, errOut := f.run(t, "src/svc/v2.0.0", true); code != 0 { + t.Fatalf("block form must still bump, got %d\n%s%s", code, out, errOut) + } + if got := f.read(t, "c", "values.yaml"); !strings.Contains(got, `tag: "2.0.0"`) { + t.Fatalf("image tag did not move:\n%s", got) + } +} From ddb1aa7390279dfd0c286bd12356c530ad7407a9 Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Sat, 29 Aug 2026 11:29:39 -0700 Subject: [PATCH 9/9] fix(ci): refuse an inline image wherever the key appears on the line The inline check was anchored to the start of the line, so it only caught the simplest form. These are valid YAML, hide the tag from a line scan, and all reached ActionAppVersionOnly: app: { image: { tag: "1.0.0" } } - image: { tag: "1.0.0" } appVersion moved on its own and the deployed image stayed where it was. This is the fourth shape of one bug, so the rule is inverted rather than extended again. Instead of enumerating the forms that hide a tag, anything that is not a plain block image: is refused, wherever the key sits on the line. Adding a fifth shape now needs no code change. The prefix must end at a space, { or , so a colon inside a value such as repository: myimage:1.0.0 is not read as an image key, and # is excluded so commented lines do not trip it. No chart changes behaviour: all eleven charts with a declared service edge plan exactly as before. The only two files the rule newly matches are deploy/helm/openbao and deploy/helm/nvcf-unbound, which declare an image as a scalar and are not reachable from a service release today. Mutation checked: restoring the line-anchored regex is caught. Co-authored-by: Balaji Ganesan Signed-off-by: Balaji Ganesan --- tools/chart-version-bumper/chart.go | 19 ++++++++++++++++--- tools/chart-version-bumper/main_test.go | 5 +++++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/tools/chart-version-bumper/chart.go b/tools/chart-version-bumper/chart.go index 4e0f8fb56..fc8228dc7 100644 --- a/tools/chart-version-bumper/chart.go +++ b/tools/chart-version-bumper/chart.go @@ -42,9 +42,22 @@ var ( appVersionRE = regexp.MustCompile(`(?m)^(appVersion:\s*)"?([^"\s#]+)"?(.*)$`) tagLineRE = regexp.MustCompile(`^(\s+)tag:\s*"?([^"\s#]*)"?`) // An image: key carrying a value on the same line, rather than opening a - // block. Covers a flow mapping, image: { tag: "1.0.0" }, and a plain scalar - // reference, image: registry/name:tag. - inlineImageRE = regexp.MustCompile(`(?m)^\s*image:[ \t]*[^ \t\n#].*$`) + // block, wherever it appears on that line. + // + // Anchoring to the start of the line only caught the simplest form. These + // are all valid YAML and all hide the tag from a line scan: + // + // image: { tag: "1.0.0" } + // app: { image: { tag: "1.0.0" } } + // - image: { tag: "1.0.0" } + // image: registry/name:tag + // + // So the rule is inverted: rather than enumerate the shapes that hide a tag, + // anything that is not a plain block image: is refused. The optional prefix + // must end at a space, { or , so that a colon inside a value, such as + // repository: myimage:1.0.0, is not mistaken for an image key, and excluding + // # keeps commented lines out. + inlineImageRE = regexp.MustCompile(`(?m)^(?:[^#\n]*[\s{,])?image:[ \t]*[^ \t\n#].*$`) keyLineRE = regexp.MustCompile(`^(\s*)([A-Za-z0-9_.-]+):`) ) diff --git a/tools/chart-version-bumper/main_test.go b/tools/chart-version-bumper/main_test.go index 90613fc60..0d1c2a1e7 100644 --- a/tools/chart-version-bumper/main_test.go +++ b/tools/chart-version-bumper/main_test.go @@ -495,6 +495,11 @@ func TestInlineImageMappingRefusesRatherThanHalfBumping(t *testing.T) { `image: { tag: "1.0.0" }`, `image: registry.example/app:1.0.0`, "app:\n image: { repository: \"r\", tag: \"1.0.0\" }", + // An image: key that does not start its line. Anchoring the check to the + // line start missed both of these, and each one reached + // ActionAppVersionOnly and half-bumped the chart. + `app: { image: { tag: "1.0.0" } }`, + "containers:\n - image: { tag: \"1.0.0\" }", } { f := newFixture(t, `{"services":[ {"id":"svc","path":"src/svc"},