fix(release): let a stack declare the image pins it repeats - #1987
Open
kristinapathak wants to merge 1 commit into
Open
kristinapathak wants to merge 1 commit into
kristinapathak wants to merge 1 commit into
Conversation
deploy/stacks/nvcf-compute-plane pins the BYOO OTel collector in environments/base.yaml, the same image the nvca-operator chart pins. The chart's four fields auto-bump on every collector release through their deploys edge; the stack had no edge and no way to declare one, so it sat on 0.157.0-nv-0.2.1 while the chart moved to 0.160.0-nv-0.2.5. The gap widened with each release rather than holding steady. Declaring the edge was not enough on its own. Two lookups excluded the stack before it could be planned: - ChartsDeploying required deploy/helm/, so a deploy/stacks/ entry was never a candidate at all, and the run reported "no chart declares that it deploys byoo-otel-collector". - PlanForValuesPaths then required a Chart.yaml under the entry path. A stack is a helmfile tree with no chart of its own, and its charts sit at charts/*/Chart.yaml, two levels below where ChartFiles looks. Both now make an exception for an edge that names its files explicitly, which is self-describing and needs no chart to resolve against. An edge using values_paths still requires a Chart.yaml, because those paths resolve against the chart's own values.yaml and there is nothing to resolve them against without one. ApplyValuesPaths needed no change: it returns through commitFileUpdates before touching Chart.yaml whenever app_version is false, which it is here. The stale pin itself is bumped in this commit by running the tool (`chart-version-bumper --tag .../v0.160.0-nv-0.2.5 --write`) rather than by editing the file, so the fix and the mechanism are the same code path. Golden testdata carries the rendered value twice. Those two literals are substituted rather than re-rendered: rendering pulls the operator chart from nvcr.io, which needs credentials this checkout does not have. The change is literal-for-literal what a re-render produces, and no workflow runs compare-golden. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
kristinapathak
added
the
ci
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NVIDIA/nvcf/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (5)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe chart-version bumper now supports eligible chartless stack targets that update declared values files. Release configuration connects the OTEL collector to the stack, and the stack pin advances to ChangesChartless stack bump
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant CollectorRelease
participant chart-version-bumper
participant base.yaml
CollectorRelease->>chart-version-bumper: publish byoo-otel-collector version
chart-version-bumper->>base.yaml: update declared OTEL image tag
base.yaml-->>chart-version-bumper: record updated stack value
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The chartless values-file support and collector release wiring have no identified actionable issue. The change is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
deploy/stacks/nvcf-compute-planerepeats the collector pin thenvca-operatorchart carries, but had no way to declare that. The chart auto-bumped to
0.160.0-nv-0.2.5; the stack sat on0.157.0-nv-0.2.1. It can declare it now,and this bumps it.
Before, on
main:After:
Additional Details
Declaring the edge is not enough on its own. That was the first thing I
tried, and it silently did nothing. Two lookups exclude a stack before it can be
planned:
ChartsDeployingrequiresdeploy/helm/, so adeploy/stacks/entry is nevera candidate — hence the
no chart declares…line above.PlanForValuesPathsthen requires aChart.yamlunder the entry path. A stackis a helmfile tree with no chart of its own, and its charts sit at
charts/*/Chart.yaml, two levels below whereChartFilesglobs.Both now make an exception for an edge that names its files explicitly via
values_files. That form is self-describing: it says which file and which dottedpath, so it needs no chart to resolve against.
An edge using
values_pathsstill requires aChart.yaml, because thosepaths resolve against the chart's own
values.yamland there is nothing toresolve them against without one. There's a test pinning that, so the exception
can't quietly widen into "charts are optional."
ApplyValuesPathsneeded no change — it returns throughcommitFileUpdatesbefore touching
Chart.yamlwheneverapp_versionis false, which it is here.The stale pin is bumped by running the tool, not by editing the file:
chart-version-bumper --tag .../v0.160.0-nv-0.2.5 --write. The fix and themechanism that prevents recurrence are the same code path, so the one-line diff
in
base.yamlis also the end-to-end proof.Why this drifted and
deploy/stacks/self-manageddidn't: that stack iscovered by different machinery —
stack-pin-bump.yml/stack-pin-resolver,scoped to
deploy/stacks/self-managed/helmfile.d. Nothing covered this one.For the Reviewer
The two guard relaxations in
metadata.goandchart.goare the substance;please sanity-check that keying the exception on
len(d.ValuesFiles) == 0is theboundary you'd want, rather than an explicit
kind: stackmarker on the entry.I chose the former because it derives from what the edge already has to say, and
adds no new vocabulary.
For QA
go test ./...intools/chart-version-bumperpasses, 2 tests new. Bothconfirmed failing first — the values_files one left the pin at
0.157.0-nv-0.2.1.--write; outputabove. The resulting
base.yamldiff is one line.tools/ci/check-go-tools: 10 modules, 9 tested, 0 failed.tools/ci/chart-service-edge --audit: 25 charts, 21 declared, 4 undeclared,0 naming an unknown service — unchanged from
main.tools/ci/stack-pin-resolver --audit: 31 releases, 0 unresolved.docs-version-syncfails in a checkout missing recenttags (
resolve stack source tag …/v0.20.7). It fails identically on cleanmainand passes aftergit fetch --tags. Not related to this change.Golden testdata carries the rendered value twice. Those two literals are
substituted, not re-rendered — rendering pulls the operator chart from
nvcr.ioand needs credentials this checkout lacks. The substitution isliteral-for-literal what a re-render produces for a value-only change, and no
workflow runs
compare-golden(nothing in.github/workflowsinvokescompare-golden,test-localorrender-local). Flagging it so a reviewer withregistry access can confirm rather than take my word for it.
Not exercised: a real release-triggered
chart-version-bump.ymlrun.Issues
Closes #1986
Checklist
🤖 Generated with Claude Code
Summary by CodeRabbit
Updates
0.160.0-nv-0.2.5.Bug Fixes