From a27994385591a51a97c85372cb5737fc3b976a50 Mon Sep 17 00:00:00 2001 From: Kristina Pathak Date: Tue, 27 Oct 2026 23:35:15 -0700 Subject: [PATCH 1/2] fix(release): open stack trains with a mergeable bump and an inventory branch-cut built the next-train VERSION bump on the release branch snapshot. The snapshot copies the default branch tree onto a parent from before the first merge in its history, so the bump shared no recent history with the default branch. Its pull request conflicted and listed the whole repository as changed. Build the bump on the selected default-branch commit instead, on both the create and reuse paths. That history includes the default branch's merges, so the bump branch moves from release-bump/ to chore/release-train/, outside the release-* names that the release ruleset holds to a linear history. The tag workflow's stack inventory requires every pinned source chart tag to be an ancestor of the stack commit. Chart tags released from the default branch are never ancestors of a snapshot, so the first release of a train rooted at a snapshot could not produce its inventory. The snapshot now records the selected commit in a Snapshot-Of trailer. The inventory also accepts a chart tag that is an ancestor of that commit, after confirming the commit holds the snapshot's tree. A snapshot cut before the trailer existed is matched to a commit with the same tree. A chart tag outside both histories is still rejected. Fixes #2970 Signed-off-by: Kristina Pathak Co-Authored-By: Claude Opus 5.5 --- docs/dev/github-release-process.md | 15 ++ tools/ci/github-release | 23 ++- tools/ci/test-github-release.py | 113 ++++++++++++++- .../resolved_inventory_publish.go | 116 ++++++++++++++- .../resolved_inventory_publish_test.go | 133 ++++++++++++++++++ 5 files changed, 392 insertions(+), 8 deletions(-) diff --git a/docs/dev/github-release-process.md b/docs/dev/github-release-process.md index 8458e99d95..9f5027bd64 100644 --- a/docs/dev/github-release-process.md +++ b/docs/dev/github-release-process.md @@ -404,6 +404,21 @@ release branch is a parallel history: its tags are not ancestors of `main`, which is why `release_baseline_version` filters on reachability. +The snapshot records the selected default-branch commit in a +`Snapshot-Of: ` trailer, and that commit must hold the snapshot's +tree. Chart tags released from the default branch are ancestors of that +commit but not of the snapshot. The stack inventory accepts a pinned +source chart tag that is an ancestor of the stack commit or of the +recorded commit. A snapshot cut before the trailer existed is matched to +a commit in the checkout with the same tree. + +The `VERSION` bump pull request is a single commit on top of the +selected default-branch commit, pushed to +`chore/release-train//vX.Y-to-vX.Y+1`. It carries the +default branch's history, including its merges, so it stays outside the +`release-*` branch names that the release ruleset holds to a linear +history. + Re-running `branch-cut` is safe. An existing release branch whose tree matches the selected base is reused, one whose tree differs is an error, and an open bump pull request is reported rather than replaced. diff --git a/tools/ci/github-release b/tools/ci/github-release index 03695a575c..2fe17d9f25 100755 --- a/tools/ci/github-release +++ b/tools/ci/github-release @@ -299,8 +299,14 @@ def release_branch_train(service, branch, root=None): def service_version_bump_branch(service, version): + """The branch that carries the default-branch VERSION bump for a new train. + + It sits on the default branch's history, which holds merges, so it stays + outside the `release-*` namespace whose ruleset requires a merge-free + history. + """ major, minor, _patch = [int(part) for part in version.split(".")] - return f"release-bump/{service['id']}/v{major}.{minor}-to-v{major}.{minor + 1}" + return f"chore/release-train/{service['id']}/v{major}.{minor}-to-v{major}.{minor + 1}" def next_release_train_version(version): @@ -327,6 +333,11 @@ def linear_release_branch_base(root, base_sha): release branch is a parallel history: its tags are not ancestors of the default branch, which is why `release_baseline_version` filters on reachability. + + The snapshot records base_sha in a `Snapshot-Of` trailer. Chart tags + released from the default branch are ancestors of base_sha but not of the + snapshot, and the stack inventory reads the trailer to prove each pinned + chart was released from source the stack contains. """ merges = run( ["git", "rev-list", "--topo-order", "--reverse", "--merges", base_sha], @@ -351,6 +362,8 @@ def linear_release_branch_base(root, base_sha): linear_parent, "-m", "chore(release): snapshot default branch for linear history", + "-m", + f"Snapshot-Of: {base_sha}", ], cwd=root, capture=True, @@ -1891,13 +1904,11 @@ def branch_cut(args): f"not selected base tree {base_tree}" ) print(f"[github-release] {release_branch} already has the selected base tree") - bump_base = existing_release_sha else: ensure_git_identity(root) release_base = linear_release_branch_base(root, base_sha) run(["git", "push", "origin", f"{release_base}:refs/heads/{release_branch}"], cwd=root) print(f"[github-release] created {release_branch} from selected base {base_sha}") - bump_base = release_base existing_pr = open_pr_url(root, bump_branch, target_branch) if existing_pr: @@ -1907,8 +1918,12 @@ def branch_cut(args): if remote_branch_sha(root, bump_branch): raise SystemExit(f"{bump_branch} already exists but has no open PR to {target_branch}") + # The bump targets the default branch, so it sits on the commit that was + # cut, not on the release branch snapshot. On the snapshot it would share + # no recent history with the default branch and its pull request would + # conflict. ensure_git_identity(root) - run(["git", "switch", "-C", bump_branch, bump_base], cwd=root) + run(["git", "switch", "-C", bump_branch, base_sha], cwd=root) version_path = root / version_file_path version_path.write_text(f"{next_version}\n") run(["git", "add", version_file_path], cwd=root) diff --git a/tools/ci/test-github-release.py b/tools/ci/test-github-release.py index b187906b02..c4f7556c4a 100644 --- a/tools/ci/test-github-release.py +++ b/tools/ci/test-github-release.py @@ -1279,7 +1279,7 @@ def test_branch_cut_dry_run_reports_release_branch_and_bump_pr(self): text = output.getvalue() self.assertIn("release-deploy/stacks/self-managed/v0.21", text) - self.assertIn("release-bump/nvcf-self-managed-stack/v0.21-to-v0.22", text) + self.assertIn("chore/release-train/nvcf-self-managed-stack/v0.21-to-v0.22", text) self.assertIn("deploy/stacks/self-managed/VERSION=0.21.0->0.22.0", text) def test_branch_cut_requires_a_release_branch_only_service(self): @@ -1301,6 +1301,104 @@ def test_branch_cut_requires_a_release_branch_only_service(self): with chdir(root), self.assertRaisesRegex(SystemExit, "requires release.release_branch_only"): self.github_release.branch_cut(args) + def _seed_default_branch_with_a_merge(self, root, remote, version): + """A stack repo whose default branch history holds a merge, pushed to a bare origin.""" + self._seed_stack_release_branch(root, version, "") + main_branch = git_out(root, "branch", "--show-current").strip() + git(root, "switch", "-c", "merged-change") + (root / "merged.txt").write_text("merged change\n") + self.commit_all(root, "fix: merged change") + git(root, "switch", main_branch) + (root / "main.txt").write_text("main change\n") + self.commit_all(root, "fix: main change") + git(root, "merge", "--no-ff", "merged-change", "-m", "Merge merged-change") + init_bare_remote(remote) + git(root, "remote", "add", "origin", str(remote)) + git(root, "push", "origin", "HEAD:refs/heads/main") + return git_out(root, "rev-parse", "HEAD").strip() + + def fake_branch_cut_pull_requests(self): + """Answer `gh pr list` with no open pull request and record `gh pr create`.""" + created = [] + real_run = subprocess.run + + def fake_run(args, *rest, **kwargs): + if list(args[:3]) == ["gh", "pr", "list"]: + return subprocess.CompletedProcess(args, 0, stdout="[]") + if list(args[:3]) == ["gh", "pr", "create"]: + created.append(list(args)) + return subprocess.CompletedProcess(args, 0, stdout="") + return real_run(args, *rest, **kwargs) + + self.github_release.subprocess = SubprocessShim(fake_run) + return created + + def test_branch_cut_builds_the_version_bump_on_the_selected_default_branch_commit(self): + # The release branch is a merge-free copy of the selected tree. The + # VERSION bump targets the default branch, so it must be one commit on + # top of the commit that was cut. Built on the release branch snapshot, + # it shares no recent history with the default branch and its pull + # request conflicts and lists the whole repository as changed. + release_branch = "release-deploy/stacks/self-managed/v1.1" + bump_branch = "chore/release-train/nvcf-self-managed-stack/v1.1-to-v1.2" + version_path = "deploy/stacks/self-managed/VERSION" + for release_branch_exists in (False, True): + with self.subTest(release_branch_exists=release_branch_exists), tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) / "repo" + remote = Path(tmp) / "remote.git" + root.mkdir() + base_sha = self._seed_default_branch_with_a_merge(root, remote, "1.1.0") + if release_branch_exists: + snapshot = self.github_release.linear_release_branch_base(root, base_sha) + git(root, "push", "origin", f"{snapshot}:refs/heads/{release_branch}") + created = self.fake_branch_cut_pull_requests() + metadata_path = Path(tmp) / "metadata.json" + metadata_path.write_text( + json.dumps({"version": 1, "services": [self.SELF_MANAGED_STACK_SERVICE]}) + ) + + args = types.SimpleNamespace( + metadata=str(metadata_path), + service="nvcf-self-managed-stack", + ref=base_sha, + target_branch="main", + dry_run=False, + ) + with chdir(root), contextlib.redirect_stdout(io.StringIO()): + self.github_release.branch_cut(args) + + bump_head = git_out(remote, "rev-parse", f"refs/heads/{bump_branch}").strip() + self.assertEqual(git_out(remote, "rev-parse", f"{bump_head}^").strip(), base_sha) + self.assertEqual(git_out(remote, "rev-list", f"{base_sha}..{bump_head}").split(), [bump_head]) + self.assertEqual(git_out(remote, "diff", "--name-only", base_sha, bump_head).strip(), version_path) + self.assertEqual(git_out(remote, "show", f"{bump_head}:{version_path}"), "1.2.0\n") + self.assertEqual(len(created), 1) + self.assertEqual(created[0][created[0].index("--head") + 1], bump_branch) + self.assertEqual(created[0][created[0].index("--base") + 1], "main") + + # The release branch itself still holds the selected tree on a + # merge-free history, and records the commit it copies. + release_head = git_out(remote, "rev-parse", f"refs/heads/{release_branch}").strip() + self.assertEqual( + git_out(remote, "rev-parse", f"{release_head}^{{tree}}"), + git_out(remote, "rev-parse", f"{base_sha}^{{tree}}"), + ) + self.assertEqual(git_out(remote, "rev-list", "--merges", release_head).strip(), "") + self.assertEqual( + git_out( + remote, "log", "-1", "--format=%(trailers:key=Snapshot-Of,valueonly)", release_head + ).strip(), + base_sha, + ) + + def test_version_bump_branch_is_outside_the_release_branch_namespace(self): + # Release branches require a merge-free history. The bump branch + # descends from the default branch, which holds merges, so it must not + # live under a release-* name. + bump_branch = self.github_release.service_version_bump_branch(self.SELF_MANAGED_STACK_SERVICE, "1.1.0") + self.assertEqual(bump_branch, "chore/release-train/nvcf-self-managed-stack/v1.1-to-v1.2") + self.assertFalse(bump_branch.startswith("release-")) + def test_version_file_must_hold_a_stable_version(self): # A prerelease here would be cut verbatim as the train's first release. with tempfile.TemporaryDirectory() as tmp: @@ -1336,6 +1434,17 @@ def test_linear_release_branch_base_preserves_the_selected_tree(self): self.github_release.commit_tree(root, release_base), self.github_release.commit_tree(root, base_sha), ) + # The stack inventory reads this trailer to accept chart tags + # released from the default branch, which are not ancestors of + # the snapshot. + self.assertEqual( + git_out(root, "log", "-1", "--format=%(trailers:key=Snapshot-Of,valueonly)", release_base).strip(), + base_sha, + ) + self.assertEqual( + git_out(root, "log", "-1", "--format=%s", release_base).strip(), + "chore(release): snapshot default branch for linear history", + ) self.assertEqual( self.github_release.run( ["git", "rev-list", "--merges", release_base], cwd=root, capture=True @@ -1447,7 +1556,7 @@ def test_the_stacks_release_from_a_branch_and_nothing_else_does(self): self.github_release.service_release_branch(service, version, root).startswith("release-") ) self.assertTrue( - self.github_release.service_version_bump_branch(service, version).startswith("release-bump/") + self.github_release.service_version_bump_branch(service, version).startswith("chore/release-train/") ) def test_nvca_still_releases_from_main(self): diff --git a/tools/docs-version-sync/resolved_inventory_publish.go b/tools/docs-version-sync/resolved_inventory_publish.go index fe429eb7a2..9e3fe66723 100644 --- a/tools/docs-version-sync/resolved_inventory_publish.go +++ b/tools/docs-version-sync/resolved_inventory_publish.go @@ -829,6 +829,7 @@ func materializeResolvedInventorySourceCharts( if err != nil { return nil, fmt.Errorf("parse releases for source chart rendering: %w", err) } + lineage := &stackSourceLineage{repoRoot: repoRoot, stackCommit: stackSource.Commit} var used []string for _, release := range releases { alias, name, found := strings.Cut(release.Chart, "/") @@ -852,8 +853,12 @@ func materializeResolvedInventorySourceCharts( } return nil, fmt.Errorf("source chart %s path %s is unavailable at %s: %w", name, sourceChart.Path, tag, err) } - if _, err := gitOutput(repoRoot, "merge-base", "--is-ancestor", tag, stackSource.Commit); err != nil { - return nil, fmt.Errorf("source chart %s tag %s is not an ancestor of stack commit %s: %w", name, tag, stackSource.Commit, err) + inSource, err := lineage.contains(tag) + if err != nil { + return nil, fmt.Errorf("source chart %s: %w", name, err) + } + if !inSource { + return nil, fmt.Errorf("source chart %s tag %s is not an ancestor of stack commit %s%s", name, tag, stackSource.Commit, lineage.describeOrigins()) } chartPath := filepath.Join(chartRoot, name, release.Version) if err := extractResolvedInventoryGitChart(repoRoot, tag, sourceChart.Path, chartPath); err != nil { @@ -870,6 +875,113 @@ func materializeResolvedInventorySourceCharts( return used, nil } +const ( + // releaseSnapshotSubject is the subject tools/ci/github-release gives the + // commit that roots a stack release branch. That commit copies a + // default-branch tree onto a merge-free parent, so chart tags released + // from the default branch are not its ancestors. + releaseSnapshotSubject = "chore(release): snapshot default branch for linear history" + // releaseSnapshotTrailer names the default-branch commit a snapshot copies. + releaseSnapshotTrailer = "Snapshot-Of" +) + +var gitCommitNamePattern = regexp.MustCompile(`^[0-9a-f]{7,64}$`) + +// stackSourceLineage answers whether a chart tag was released from source +// that a stack commit contains. +type stackSourceLineage struct { + repoRoot string + stackCommit string + origins []string + loaded bool +} + +// contains reports whether tag is an ancestor of the stack commit, or of the +// default-branch commit that the stack's release branch snapshot copied. +func (lineage *stackSourceLineage) contains(tag string) (bool, error) { + if _, err := gitOutput(lineage.repoRoot, "merge-base", "--is-ancestor", tag, lineage.stackCommit); err == nil { + return true, nil + } + if !lineage.loaded { + origins, err := releaseSnapshotOrigins(lineage.repoRoot, lineage.stackCommit) + if err != nil { + return false, err + } + lineage.origins, lineage.loaded = origins, true + } + for _, origin := range lineage.origins { + if _, err := gitOutput(lineage.repoRoot, "merge-base", "--is-ancestor", tag, origin); err == nil { + return true, nil + } + } + return false, nil +} + +func (lineage *stackSourceLineage) describeOrigins() string { + if len(lineage.origins) == 0 { + return "" + } + return " or of its release snapshot source " + strings.Join(lineage.origins, ", ") +} + +// releaseSnapshotOrigins returns the commits whose source the release branch +// snapshot under stackCommit copied, or nothing when stackCommit does not +// descend from a snapshot. +// +// A snapshot names its origin in a Snapshot-Of trailer, and that commit must +// hold the snapshot's tree. A snapshot cut before the trailer existed has +// none; any other commit in the checkout with the snapshot's exact tree holds +// the same source. +func releaseSnapshotOrigins(repoRoot, stackCommit string) ([]string, error) { + raw, err := gitOutput(repoRoot, "log", "--fixed-strings", "--grep="+releaseSnapshotSubject, "--format=%H%x00%T%x00%s", stackCommit) + if err != nil { + return nil, fmt.Errorf("find release snapshot under stack commit %s: %w", stackCommit, err) + } + var snapshot, snapshotTree string + for _, line := range strings.Split(trimOutput(raw), "\n") { + fields := strings.SplitN(line, "\x00", 3) + if len(fields) == 3 && fields[2] == releaseSnapshotSubject { + snapshot, snapshotTree = fields[0], fields[1] + break + } + } + if snapshot == "" { + return nil, nil + } + + raw, err = gitOutput(repoRoot, "log", "-1", "--format=%(trailers:key="+releaseSnapshotTrailer+",valueonly)", snapshot) + if err != nil { + return nil, fmt.Errorf("read release snapshot %s: %w", snapshot, err) + } + if recorded := strings.Fields(trimOutput(raw)); len(recorded) > 0 { + origin := recorded[0] + if len(recorded) != 1 || !gitCommitNamePattern.MatchString(origin) { + return nil, fmt.Errorf("release snapshot %s has an invalid %s trailer %q", snapshot, releaseSnapshotTrailer, trimOutput(raw)) + } + tree, err := gitOutput(repoRoot, "rev-parse", "--verify", origin+"^{tree}") + if err != nil { + return nil, fmt.Errorf("resolve %s commit %s of release snapshot %s: %w", releaseSnapshotTrailer, origin, snapshot, err) + } + if trimOutput(tree) != snapshotTree { + return nil, fmt.Errorf("release snapshot %s does not hold the tree of its %s commit %s", snapshot, releaseSnapshotTrailer, origin) + } + return []string{origin}, nil + } + + raw, err = gitOutput(repoRoot, "log", "--all", "--format=%H %T") + if err != nil { + return nil, fmt.Errorf("find commits with the tree of release snapshot %s: %w", snapshot, err) + } + var origins []string + for _, line := range strings.Split(trimOutput(raw), "\n") { + commit, tree, found := strings.Cut(line, " ") + if found && tree == snapshotTree && commit != snapshot { + origins = append(origins, commit) + } + } + return origins, nil +} + func extractResolvedInventoryGitChart(repoRoot, tag, sourcePath, destination string) error { raw, err := gitOutput(repoRoot, "archive", "--format=tar", tag, "--", sourcePath) if err != nil { diff --git a/tools/docs-version-sync/resolved_inventory_publish_test.go b/tools/docs-version-sync/resolved_inventory_publish_test.go index c21d673552..84cfc53eb2 100644 --- a/tools/docs-version-sync/resolved_inventory_publish_test.go +++ b/tools/docs-version-sync/resolved_inventory_publish_test.go @@ -157,6 +157,139 @@ func TestMaterializeResolvedInventorySourceChartsAllowsUnavailableHistoricalPath } } +// TestMaterializeResolvedInventorySourceChartsFollowsReleaseSnapshot covers a +// stack release branch rooted at a snapshot: a commit that copies a +// default-branch tree onto a merge-free parent. Chart tags released from the +// default branch are ancestors of the copied commit, not of the snapshot. +func TestMaterializeResolvedInventorySourceChartsFollowsReleaseSnapshot(t *testing.T) { + repo := initTestGitRepo(t) + git := func(args ...string) string { + t.Helper() + output, err := gitOutput(repo, args...) + if err != nil { + t.Fatal(err) + } + return trimOutput(output) + } + commitFiles := func(message string, files map[string]string) string { + t.Helper() + for name, body := range files { + writeFile(t, filepath.Join(repo, filepath.FromSlash(name)), body) + } + git("add", "--all") + git("commit", "-m", message) + return git("rev-parse", "HEAD") + } + + const statePath = "deploy/stacks/self-managed/helmfile.d/state.yaml.gotmpl" + const chartMetadata = "apiVersion: v2\nname: helm-example\nversion: 0.0.0\n" + stateBody := "releases:\n - name: example\n chart: nvcf/helm-example\n version: 1.0.0\n" + sourceCharts := map[string]resolvedInventorySourceChart{ + "helm-example": {TagPrefix: "deploy/helm/example/v", Path: "deploy/helm/example"}, + } + + linearParent := commitFiles("chore: seed", map[string]string{"README.md": "seed\n"}) + commitFiles("feat: example chart", map[string]string{"deploy/helm/example/Chart.yaml": chartMetadata}) + git("tag", "deploy/helm/example/v1.0.0") + base := commitFiles("feat: pin example chart", map[string]string{statePath: stateBody}) + baseTree := git("rev-parse", base+"^{tree}") + + // A chart released from a commit the stack never contained. + sideChart := git("commit-tree", git("rev-parse", "deploy/helm/example/v1.0.0^{tree}"), "-p", linearParent, "-m", "feat: unmerged chart") + git("tag", "deploy/helm/example/v2.0.0", sideChart) + + snapshot := func(parent string, paragraphs ...string) string { + args := []string{"commit-tree", baseTree, "-p", parent} + for _, paragraph := range paragraphs { + args = append(args, "-m", paragraph) + } + return git(args...) + } + recorded := snapshot(linearParent, releaseSnapshotSubject, releaseSnapshotTrailer+": "+base) + legacy := snapshot(linearParent, releaseSnapshotSubject) + mismatched := snapshot(linearParent, releaseSnapshotSubject, releaseSnapshotTrailer+": "+linearParent) + patch := snapshot(recorded, "fix(stack): backport a change") + + materialize := func(t *testing.T, stackCommit, chartVersion string) ([]string, error) { + t.Helper() + stateFile := filepath.Join(t.TempDir(), "state.yaml.gotmpl") + writeFile(t, stateFile, stateBody) + fullRaw := mustJSON(t, []helmfileRelease{{Name: "example", Chart: "nvcf/helm-example", Version: chartVersion}}) + return materializeResolvedInventorySourceCharts( + repo, + stateFile, + t.TempDir(), + stackSourceRelease{Version: "1.1.0", Tag: "deploy/stacks/self-managed/v1.1.0", Commit: stackCommit}, + fullRaw, + sourceCharts, + false, + ) + } + + accepted := []struct { + name string + stackCommit string + }{ + {name: "default branch commit", stackCommit: base}, + {name: "snapshot with Snapshot-Of trailer", stackCommit: recorded}, + {name: "patch on a snapshot branch", stackCommit: patch}, + {name: "snapshot without trailer matched by tree", stackCommit: legacy}, + } + for _, test := range accepted { + t.Run(test.name, func(t *testing.T) { + used, err := materialize(t, test.stackCommit, "1.0.0") + if err != nil { + t.Fatalf("materialize source charts: %v", err) + } + if !reflect.DeepEqual(used, []string{"helm-example"}) { + t.Fatalf("used source charts = %v, want [helm-example]", used) + } + }) + } + + rejected := []struct { + name string + stackCommit string + chartVersion string + wantErr []string + }{ + { + name: "chart tag outside a default branch stack", + stackCommit: base, + chartVersion: "2.0.0", + wantErr: []string{"tag deploy/helm/example/v2.0.0 is not an ancestor of stack commit " + base}, + }, + { + name: "chart tag outside a snapshot's source", + stackCommit: recorded, + chartVersion: "2.0.0", + wantErr: []string{ + "tag deploy/helm/example/v2.0.0 is not an ancestor of stack commit " + recorded, + "or of its release snapshot source " + base, + }, + }, + { + name: "Snapshot-Of commit with another tree", + stackCommit: mismatched, + chartVersion: "1.0.0", + wantErr: []string{"does not hold the tree of its Snapshot-Of commit " + linearParent}, + }, + } + for _, test := range rejected { + t.Run(test.name, func(t *testing.T) { + _, err := materialize(t, test.stackCommit, test.chartVersion) + if err == nil { + t.Fatal("materialize source charts succeeded, want an ancestry error") + } + for _, want := range test.wantErr { + if !strings.Contains(err.Error(), want) { + t.Fatalf("error = %v, want %q", err, want) + } + } + }) + } +} + func TestCollectResolvedStackInventoryBuildsConfiguredSelfManagedStates(t *testing.T) { t.Setenv("NVCF_RELEASE_NGC_API_KEY", "test-api-key") t.Setenv("NVCF_RELEASE_HELM_REGISTRY", "registry.example.test/release/charts") From c61f9023a8740caac0c14f2bbb5feafe804c991c Mon Sep 17 00:00:00 2001 From: Kristina Pathak Date: Wed, 28 Oct 2026 00:00:20 -0700 Subject: [PATCH 2/2] fix(release): render stack inventories that pin a chart released after the cut The stack inventory renders each source chart from its release tag and requires that tag to be in the stack's history, so the chart source it renders is reviewed source and not whatever commit a tag points at. A patch on a stack release branch that moves a pin to a chart version released after the train was cut fails that check: the new tag comes from the default branch or from a chart maintenance branch, neither of which the train contains. The tag workflow then stops before it creates the stack's GitHub Release. When a pinned chart tag is outside the stack's history, accept it if origin/main or the chart's maintenance branch for the tag's minor, origin/release-X.Y, contains it. The tag workflow's full-history checkout fetches every branch. A tag on any other branch, including another chart's maintenance branch or the wrong minor's, is still rejected. The chart path and Chart.yaml name checks at the tag are unchanged, and the inventory schema is unchanged. The render log names the branch that supplied the chart. Fixes #2972 Signed-off-by: Kristina Pathak Co-Authored-By: Claude Opus 5.5 --- docs/dev/github-release-process.md | 11 +- .../resolved_inventory_publish.go | 34 +++- .../resolved_inventory_publish_test.go | 155 ++++++++++++++++++ 3 files changed, 198 insertions(+), 2 deletions(-) diff --git a/docs/dev/github-release-process.md b/docs/dev/github-release-process.md index 9f5027bd64..859b176d05 100644 --- a/docs/dev/github-release-process.md +++ b/docs/dev/github-release-process.md @@ -337,7 +337,9 @@ recover a failed release-note step without moving the immutable source tag. Automatic chart and stack pin updates skip releases with a lower major, minor, or patch than the selected pin. A patch for an older minor must not downgrade the default branch. Update maintenance stack pins through their own reviewed -Pull Requests after the component artifacts are verified. +Pull Requests after the component artifacts are verified. A stack inventory +renders a source chart from its release tag, so keep a chart maintenance +branch after tagging from it; the inventory looks for the tag there. Run the affected stack checks before merging its maintenance branch. These merges can create stable GitHub source releases and inventories independently @@ -412,6 +414,13 @@ source chart tag that is an ancestor of the stack commit or of the recorded commit. A snapshot cut before the trailer existed is matched to a commit in the checkout with the same tree. +A patch on the train can also pin a chart version released after the +cut. The inventory accepts a chart tag outside the stack's history when +`origin/main` or the chart's maintenance branch for that minor, +`origin/release-X.Y`, contains it. The tag workflow's +full-history checkout fetches every branch. Any other chart tag is +rejected, so the inventory still renders only reviewed chart source. + The `VERSION` bump pull request is a single commit on top of the selected default-branch commit, pushed to `chore/release-train//vX.Y-to-vX.Y+1`. It carries the diff --git a/tools/docs-version-sync/resolved_inventory_publish.go b/tools/docs-version-sync/resolved_inventory_publish.go index 9e3fe66723..13b7c730e1 100644 --- a/tools/docs-version-sync/resolved_inventory_publish.go +++ b/tools/docs-version-sync/resolved_inventory_publish.go @@ -858,7 +858,12 @@ func materializeResolvedInventorySourceCharts( return nil, fmt.Errorf("source chart %s: %w", name, err) } if !inSource { - return nil, fmt.Errorf("source chart %s tag %s is not an ancestor of stack commit %s%s", name, tag, stackSource.Commit, lineage.describeOrigins()) + branch, branches := chartReleaseBranchContaining(repoRoot, tag, sourceChart.TagPrefix, release.Version) + if branch == "" { + return nil, fmt.Errorf("source chart %s tag %s is not an ancestor of stack commit %s%s, nor of chart release branch %s", + name, tag, stackSource.Commit, lineage.describeOrigins(), strings.Join(branches, " or ")) + } + fmt.Fprintf(os.Stderr, "source chart %s tag %s is outside stack commit %s; using the release from %s\n", name, tag, stackSource.Commit, branch) } chartPath := filepath.Join(chartRoot, name, release.Version) if err := extractResolvedInventoryGitChart(repoRoot, tag, sourceChart.Path, chartPath); err != nil { @@ -982,6 +987,33 @@ func releaseSnapshotOrigins(repoRoot, stackCommit string) ([]string, error) { return origins, nil } +const ( + // chartReleaseRemote holds a remote-tracking ref for every branch in the + // release workflow's full-history checkout. + chartReleaseRemote = "origin" + chartReleaseDefaultBranch = "main" +) + +// chartReleaseBranchContaining returns the reviewed branch a chart tag was +// released from, or "" with the branches it checked. A chart releases from +// the default branch or from its maintenance branch for the tag's minor, +// release-X.Y. A stack patch can pin such a release after its +// train was cut, when the tag is not in the stack's history. +func chartReleaseBranchContaining(repoRoot, tag, tagPrefix, version string) (string, []string) { + major, rest, _ := strings.Cut(version, ".") + minor, _, _ := strings.Cut(rest, ".") + branches := []string{ + chartReleaseRemote + "/" + chartReleaseDefaultBranch, + chartReleaseRemote + "/release-" + tagPrefix + major + "." + minor, + } + for _, branch := range branches { + if _, err := gitOutput(repoRoot, "merge-base", "--is-ancestor", tag, "refs/remotes/"+branch); err == nil { + return branch, nil + } + } + return "", branches +} + func extractResolvedInventoryGitChart(repoRoot, tag, sourcePath, destination string) error { raw, err := gitOutput(repoRoot, "archive", "--format=tar", tag, "--", sourcePath) if err != nil { diff --git a/tools/docs-version-sync/resolved_inventory_publish_test.go b/tools/docs-version-sync/resolved_inventory_publish_test.go index 84cfc53eb2..0fc64dcb00 100644 --- a/tools/docs-version-sync/resolved_inventory_publish_test.go +++ b/tools/docs-version-sync/resolved_inventory_publish_test.go @@ -290,6 +290,161 @@ func TestMaterializeResolvedInventorySourceChartsFollowsReleaseSnapshot(t *testi } } +// TestMaterializeResolvedInventorySourceChartsAcceptsChartReleaseBranches +// covers a stack that pins a chart version released outside the stack's own +// history: from the default branch after the stack's train was cut, or from +// the chart's maintenance branch. The release workflow's full-history checkout +// holds a remote-tracking ref for every branch. +func TestMaterializeResolvedInventorySourceChartsAcceptsChartReleaseBranches(t *testing.T) { + repo := initTestGitRepo(t) + git := func(args ...string) string { + t.Helper() + output, err := gitOutput(repo, args...) + if err != nil { + t.Fatal(err) + } + return trimOutput(output) + } + commit := func(parent, message string, files map[string]string) string { + t.Helper() + if parent != "" { + git("checkout", "--quiet", "--detach", parent) + } + for name, body := range files { + writeFile(t, filepath.Join(repo, filepath.FromSlash(name)), body) + } + git("add", "--all") + git("commit", "--quiet", "-m", message) + return git("rev-parse", "HEAD") + } + remoteBranch := func(name, commit string) { + t.Helper() + git("update-ref", "refs/remotes/origin/"+name, commit) + } + values := func(replicas string) map[string]string { + return map[string]string{"deploy/helm/example/values.yaml": "replicas: " + replicas + "\n"} + } + + const statePath = "deploy/stacks/self-managed/helmfile.d/state.yaml.gotmpl" + stateBody := "releases:\n - name: example\n chart: nvcf/helm-example\n version: 1.0.0\n" + sourceCharts := map[string]resolvedInventorySourceChart{ + "helm-example": {TagPrefix: "deploy/helm/example/v", Path: "deploy/helm/example"}, + } + + seed := commit("", "chore: seed", map[string]string{"README.md": "seed\n"}) + chart := commit(seed, "feat: example chart", map[string]string{ + "deploy/helm/example/Chart.yaml": "apiVersion: v2\nname: helm-example\nversion: 0.0.0\n", + "deploy/helm/example/values.yaml": "replicas: 1\n", + "deploy/helm/other/Chart.yaml": "apiVersion: v2\nname: helm-other\nversion: 0.0.0\n", + }) + git("tag", "deploy/helm/example/v1.0.0") + base := commit(chart, "feat: pin example chart", map[string]string{statePath: stateBody}) + cut := git("commit-tree", git("rev-parse", base+"^{tree}"), "-p", seed, + "-m", releaseSnapshotSubject, "-m", releaseSnapshotTrailer+": "+base) + patch := git("commit-tree", git("rev-parse", cut+"^{tree}"), "-p", cut, "-m", "fix(stack): pin a newer chart") + + // Released from the default branch after the train was cut. + commit(base, "fix(chart): example 1.1.0", values("2")) + git("tag", "deploy/helm/example/v1.1.0") + git("tag", "deploy/helm/other/v1.5.0") + remoteBranch("main", commit("HEAD", "docs: later default-branch change", map[string]string{"README.md": "later\n"})) + + // Released from the chart's 1.0 maintenance branch, which also carries a + // tag that belongs to another minor. + commit(chart, "fix(chart): backport to 1.0", values("3")) + git("tag", "deploy/helm/example/v1.0.1") + commit("HEAD", "fix(chart): mislabeled release", values("4")) + git("tag", "deploy/helm/example/v1.4.0") + remoteBranch("release-deploy/helm/example/v1.0", git("rev-parse", "HEAD")) + + // Tags at commits that no release branch of the chart contains. + remoteBranch("feature/unreviewed", commit(base, "feat: unreviewed chart change", values("5"))) + git("tag", "deploy/helm/example/v1.2.0") + remoteBranch("release-deploy/helm/other/v1.3", commit(chart, "fix(other): another chart's backport", values("6"))) + git("tag", "deploy/helm/example/v1.3.0") + + materialize := func(t *testing.T, stackCommit, chartVersion, chartRoot string) ([]string, error) { + t.Helper() + stateFile := filepath.Join(t.TempDir(), "state.yaml.gotmpl") + writeFile(t, stateFile, stateBody) + fullRaw := mustJSON(t, []helmfileRelease{{Name: "example", Chart: "nvcf/helm-example", Version: chartVersion}}) + return materializeResolvedInventorySourceCharts( + repo, + stateFile, + chartRoot, + stackSourceRelease{Version: "1.1.1", Tag: "deploy/stacks/self-managed/v1.1.1", Commit: stackCommit}, + fullRaw, + sourceCharts, + false, + ) + } + + accepted := []struct { + name string + stackCommit string + chartVersion string + wantReplicas string + }{ + {name: "train patch pins a default-branch release after the cut", stackCommit: patch, chartVersion: "1.1.0", wantReplicas: "2"}, + {name: "train patch pins a chart maintenance release", stackCommit: patch, chartVersion: "1.0.1", wantReplicas: "3"}, + {name: "default-branch stack pins a chart maintenance release", stackCommit: base, chartVersion: "1.0.1", wantReplicas: "3"}, + } + for _, test := range accepted { + t.Run(test.name, func(t *testing.T) { + chartRoot := t.TempDir() + used, err := materialize(t, test.stackCommit, test.chartVersion, chartRoot) + if err != nil { + t.Fatalf("materialize source charts: %v", err) + } + if !reflect.DeepEqual(used, []string{"helm-example"}) { + t.Fatalf("used source charts = %v, want [helm-example]", used) + } + raw, err := os.ReadFile(filepath.Join(chartRoot, "helm-example", test.chartVersion, "values.yaml")) + if err != nil { + t.Fatal(err) + } + if got, want := string(raw), "replicas: "+test.wantReplicas+"\n"; got != want { + t.Fatalf("extracted values.yaml = %q, want %q from the pinned tag", got, want) + } + }) + } + + rejected := []struct { + name string + chartVersion string + wantErr string + }{ + { + name: "tag on an unmerged branch", + chartVersion: "1.2.0", + wantErr: "nor of chart release branch origin/main or origin/release-deploy/helm/example/v1.2", + }, + { + name: "tag only on another chart's maintenance branch", + chartVersion: "1.3.0", + wantErr: "nor of chart release branch origin/main or origin/release-deploy/helm/example/v1.3", + }, + { + name: "tag only on the maintenance branch of another minor", + chartVersion: "1.4.0", + wantErr: "nor of chart release branch origin/main or origin/release-deploy/helm/example/v1.4", + }, + { + name: "another chart's release tag at the pinned version", + chartVersion: "1.5.0", + wantErr: "is unavailable at deploy/helm/example/v1.5.0", + }, + } + for _, test := range rejected { + t.Run(test.name, func(t *testing.T) { + _, err := materialize(t, patch, test.chartVersion, t.TempDir()) + if err == nil || !strings.Contains(err.Error(), test.wantErr) { + t.Fatalf("error = %v, want %q", err, test.wantErr) + } + }) + } +} + func TestCollectResolvedStackInventoryBuildsConfiguredSelfManagedStates(t *testing.T) { t.Setenv("NVCF_RELEASE_NGC_API_KEY", "test-api-key") t.Setenv("NVCF_RELEASE_HELM_REGISTRY", "registry.example.test/release/charts")