Skip to content

fix(ci): umbrella appVersion computed as "---" since runner yq update - #1303

Merged
mitasovr merged 1 commit into
constructorfabric:mainfrom
mitasovr:fix/umbrella-appversion-yq
Jun 11, 2026
Merged

fix(ci): umbrella appVersion computed as "---" since runner yq update#1303
mitasovr merged 1 commit into
constructorfabric:mainfrom
mitasovr:fix/umbrella-appversion-yq

Conversation

@mitasovr

@mitasovr mitasovr commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

What broke

Every umbrella chart published since 2026-06-08 (0.1.48 → 0.1.55) carries appVersion: "---":

0.1.47 (06-04): appVersion "2026.06.04.19.35-89ba5bf"   ← last healthy
0.1.48 (06-08): appVersion "---"                         ← first broken

The runner's yq v4 update started emitting --- document separators between results when given multiple files. publish-chart's computation

NEW_APP=$(yq -r '.appVersion' <4 Chart.yaml files> | sort -V | tail -1)

then picks the separator (sort -V orders --- after date-prefixed tags). The value lands in the app.kubernetes.io/version label of every rendered resource → invalid label value → every install of the published chart fails at the pre-upgrade hook. First real-world hit: today's dev gitops deploy of 0.1.54/0.1.55 (helm release went to failed, see run history in insight-gitops).

Nobody noticed for three days because dev hadn't been deployed since June 4.

Fix

  1. Filter ---/null lines before sort -V.
  2. Fail-fast guard: abort publish if the computed appVersion doesn't match the YYYY.MM.DD.HH.MM-shortsha build-tag shape — a malformed appVersion must never ship again.
  3. Restore the committed umbrella appVersion ("---" → current max of subchart appVersions). This also makes the merge fire publish-chart (charts/insight/** path filter), so the next chart (0.1.57) publishes healthy + attested immediately.

Verified locally on yq v4.53.2: old expression → ---, fixed expression → 2026.06.11.15.07-672d487, guard passes; guard rejects ---/null/garbage.

Relation to #1290

#1290's _helpers.tpl hunk treats the committed "---" as an intentional placeholder and omits the version label when seen. Git history shows "---" is the output of this bug, not a design: it first appeared with the 0.1.48 release commit. If only #1290 merged, CI would keep publishing appVersion: "---" forever and the app.kubernetes.io/version label would silently vanish from all published charts. With this PR merged, real tags return and that guard becomes dead code (harmless; its comment should be updated or the hunk dropped).

Known-red: Run E2E suite (pre-existing cost_cents migration mismatch, tracked separately).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Chores
    • Enhanced build workflow to improve chart version extraction and validation with filtering and format verification
    • Updated umbrella chart metadata with latest version identifier

Since 2026-06-08 (charts 0.1.48 through 0.1.55) every published umbrella
chart carries appVersion "---". The runner's yq v4 update started emitting
a `---` document separator between results when invoked with multiple
files; publish-chart's NEW_APP pipeline (yq -r '.appVersion' <4 files> |
sort -V | tail -1) then picks the separator, because sort -V orders it
after the date-prefixed build tags. The value flows into the
app.kubernetes.io/version label of every rendered resource, which fails
Kubernetes label validation and breaks every install of the published
chart (first hit: dev deploy of 0.1.54/0.1.55, pre-upgrade hook rejected).

Fix the computation by filtering separator/null lines before sorting, and
fail loud unless the result matches the YYYY.MM.DD.HH.MM-shortsha build
tag shape - a malformed appVersion must never ship in a chart again.

Also restore the committed umbrella appVersion ("---" -> current max of
the four subchart appVersions). The broken value was committed back to
main by the release runs; touching charts/insight/** also makes the merge
of this PR fire publish-chart, so the next published chart is healthy and
attested without waiting for an unrelated change.

Note for reviewers of constructorfabric#1290: that PR treats the committed "---" as an
intentional placeholder and omits the version label when it is seen.
History shows "---" first appeared with the 0.1.48 release commit
(2026-06-08) as output of this bug - charts 0.1.47 and earlier carried
real build tags. With this fix merged, published charts and main's
committed Chart.yaml return to real tags, so the label guard in constructorfabric#1290
becomes dead code.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7b865c33-ea25-4415-8c98-f63bb35eb522

📥 Commits

Reviewing files that changed from the base of the PR and between 5cf7bc2 and 6eb8319.

📒 Files selected for processing (2)
  • .github/workflows/build-images.yml
  • charts/insight/Chart.yaml

📝 Walkthrough

Walkthrough

This pull request improves the umbrella Helm chart's appVersion management. The workflow now robustly computes appVersion by filtering invalid yq outputs and validates the result matches the build-tag format before application. The chart metadata is then updated with the validated version string.

Changes

Umbrella Chart appVersion Computation and Validation

Layer / File(s) Summary
Workflow appVersion filtering and validation
.github/workflows/build-images.yml
The "Bump umbrella version" step extracts .appVersion from four subcharts, filters out YAML document separators and null values produced by yq, sorts by version, and validates the result against the build-tag regex format (YYYY.MM.DD.HH.MM-shortsha) before assignment.
Chart appVersion metadata update
charts/insight/Chart.yaml
The umbrella chart's appVersion metadata is set to the validated build/version string (2026.06.11.15.07-672d487).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

A rabbit hops through version streams,
Filtering nulls and separators clean,
With regex guards to validate the way,
The chart's appVersion shines bright today! 🐰✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: fixing a CI bug where umbrella appVersion was incorrectly computed as '---' due to a yq runner update. This is the core issue addressed by the PR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@mitasovr
mitasovr merged commit e192a19 into constructorfabric:main Jun 11, 2026
12 checks passed
SharedQA added a commit to SharedQA/insight that referenced this pull request Jun 16, 2026
Review feedback on constructorfabric#1317 (cyberantonz): the guard allowed version/appVersion
forms no chart uses. Checked every Chart.yaml — `version` is always plain semver
(0.1.0 / 0.1.72) and `appVersion` is always the release build tag
(YYYY.MM.DD.HH.MM-<sha7>); no chart uses a `1.2.3-suffix` version or a
plain-semver appVersion. So:
- version regex → plain X.Y.Z (dropped the unused -/+ pre-release suffix)
- appVersion → build-tag only (dropped the dead semver branch)
Still rejects the "---" placeholder (constructorfabric#1303), which is the point. Verified the
patterns pass all five real charts and reject "---".

Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
mitasovr pushed a commit to SharedQA/insight that referenced this pull request Jul 1, 2026
The guard omitted the app.kubernetes.io/version label when appVersion was
the placeholder "---". That placeholder no longer occurs:

- constructorfabric#1303 fixed the root cause (yq emitting "---" separators) and restored a
  real committed appVersion, plus a fail-fast guard so "---" can never ship.
- constructorfabric#1431 removed dev-up.sh, the only install-from-source path that consumed
  the committed placeholder.

The condition is now unreachable dead code; this PR keeps only the
parse_descriptor.py fallback removal and its tests.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant