Skip to content

Commit 8592edc

Browse files
committed
fix(helm): match ci.yml's release predicate and resolve the version once
Two review findings, both real. The leading-whitespace tolerance was wrong here. detect-version in ci.yml is what actually cuts the tag and builds the images, and it anchors the version at the first character. Accepting a subject it rejects meant the chart could publish naming a release that was never created, pointing at images that do not exist. The pattern is now identical to ci.yml's, with a comment saying it has to stay that way. The tolerance came from a PR-title parsing problem in an earlier design; commit subjects have a different authority. The two publish jobs also resolved the version independently, and they run in parallel. A release becoming public between those two API calls would package the same immutable chart version with different appVersions, so the OCI artifact and the HTTP repo would install different Sims. Resolution moved to one job both consume, which also removes the duplicated logic. Separately, --check only inspected Chart.yaml while the script writes both that and the inventory derived from it, so it could report success over a stale inventory. It now verifies both halves; confirmed it exits 1 when the inventory is stale and 0 once regenerated.
1 parent c53f440 commit 8592edc

2 files changed

Lines changed: 61 additions & 56 deletions

File tree

‎.github/workflows/helm.yml‎

Lines changed: 47 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,49 @@ jobs:
193193
- name: Run helm test
194194
run: helm test sim --namespace sim --timeout 5m
195195

196+
# Resolved once and shared, so the two publish paths cannot package the same
197+
# immutable chart version with different appVersions -- they run in parallel,
198+
# and a release becoming public between two independent API calls would be
199+
# enough to make the OCI artifact and the HTTP repo install different Sims.
200+
release-version:
201+
name: Resolve the app release
202+
if: github.event_name == 'push' && github.ref == 'refs/heads/main' && github.repository == 'simstudioai/sim'
203+
runs-on: ${{ (vars.CI_PROVIDER == '' || vars.CI_PROVIDER == 'blacksmith') && 'blacksmith-2vcpu-ubuntu-2404' || 'ubuntu-latest' }}
204+
timeout-minutes: 5
205+
permissions:
206+
contents: read # Read the release list.
207+
outputs:
208+
version: ${{ steps.resolve.outputs.version }}
209+
steps:
210+
# On a release merge the tag does not exist yet -- this commit is what cuts
211+
# it -- so the subject is the only source available, and the latest release
212+
# is the source of truth for every other push.
213+
#
214+
# The subject pattern MUST stay identical to detect-version in ci.yml, which
215+
# is what actually creates the tag and builds the images. If this one
216+
# matched a subject that one rejects, the chart would publish naming a
217+
# release that was never cut.
218+
- name: Resolve the app release
219+
id: resolve
220+
env:
221+
HEAD_COMMIT_MESSAGE: ${{ github.event.head_commit.message }}
222+
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
223+
run: |
224+
set -euo pipefail
225+
subject=${HEAD_COMMIT_MESSAGE%%$'\n'*}
226+
if [[ "$subject" =~ ^(v[0-9]+\.[0-9]+\.[0-9]+): ]]; then
227+
resolved="${BASH_REMATCH[1]}"
228+
echo "Release commit; shipping the chart with ${resolved}."
229+
else
230+
resolved=$(gh api "repos/${GITHUB_REPOSITORY}/releases/latest" --jq .tag_name)
231+
echo "Not a release commit; shipping the chart with the latest release ${resolved}."
232+
fi
233+
if [ -z "$resolved" ]; then
234+
echo "::error::Could not resolve an app release to ship this chart with."
235+
exit 1
236+
fi
237+
echo "version=${resolved}" >> "$GITHUB_OUTPUT"
238+
196239
# Publishes the chart to GHCR as an OCI artifact. Self-hosters cannot admit a
197240
# chart pulled from a git checkout — they need an immutable, versioned artifact
198241
# they can pin by digest and mirror into an internal registry — so shipping the
@@ -204,7 +247,7 @@ jobs:
204247
# A separate workflow would race those instead of waiting for them.
205248
publish:
206249
name: Publish chart to GHCR
207-
needs: [chart, install]
250+
needs: [chart, install, release-version]
208251
if: github.event_name == 'push' && github.ref == 'refs/heads/main' && github.repository == 'simstudioai/sim'
209252
runs-on: ${{ (vars.CI_PROVIDER == '' || vars.CI_PROVIDER == 'blacksmith') && 'blacksmith-2vcpu-ubuntu-2404' || 'ubuntu-latest' }}
210253
timeout-minutes: 15
@@ -237,37 +280,12 @@ jobs:
237280
with:
238281
bun-version: 1.4.1
239282

240-
# The release this chart ships with. On a release merge the tag does not
241-
# exist yet -- it is cut by this very commit (detect-version in ci.yml) --
242-
# so the subject is the only source available, and the source of truth for
243-
# every other push is the latest release.
244-
- name: Resolve the app release
245-
id: release
246-
env:
247-
HEAD_COMMIT_MESSAGE: ${{ github.event.head_commit.message }}
248-
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
249-
run: |
250-
set -euo pipefail
251-
subject=${HEAD_COMMIT_MESSAGE%%$'\n'*}
252-
if [[ "$subject" =~ ^[[:space:]]*(v[0-9]+\.[0-9]+\.[0-9]+): ]]; then
253-
resolved="${BASH_REMATCH[1]}"
254-
echo "Release commit; shipping the chart with ${resolved}."
255-
else
256-
resolved=$(gh api "repos/${GITHUB_REPOSITORY}/releases/latest" --jq .tag_name)
257-
echo "Not a release commit; shipping the chart with the latest release ${resolved}."
258-
fi
259-
if [ -z "$resolved" ]; then
260-
echo "::error::Could not resolve an app release to ship this chart with."
261-
exit 1
262-
fi
263-
echo "version=${resolved}" >> "$GITHUB_OUTPUT"
264-
265283
# Derives appVersion rather than checking it, so the published chart cannot
266284
# be pinned to an older Sim than the release it ships with. The committed
267285
# value is kept current too, but nothing depends on a human remembering.
268286
- name: Sync chart appVersion to the release
269287
env:
270-
APP_VERSION: ${{ steps.release.outputs.version }}
288+
APP_VERSION: ${{ needs.release-version.outputs.version }}
271289
run: bun run scripts/sync-chart-appversion.ts --version "${APP_VERSION}"
272290

273291
- name: Package chart
@@ -422,7 +440,7 @@ jobs:
422440
# the job holding the signing identity.
423441
publish-http:
424442
name: Publish chart to the Helm repo
425-
needs: [chart, install]
443+
needs: [chart, install, release-version]
426444
if: github.event_name == 'push' && github.ref == 'refs/heads/main' && github.repository == 'simstudioai/sim'
427445
runs-on: ${{ (vars.CI_PROVIDER == '' || vars.CI_PROVIDER == 'blacksmith') && 'blacksmith-2vcpu-ubuntu-2404' || 'ubuntu-latest' }}
428446
timeout-minutes: 15
@@ -457,39 +475,13 @@ jobs:
457475
with:
458476
bun-version: 1.4.1
459477

460-
# The release this chart ships with. On a release merge the tag does not
461-
# exist yet -- it is cut by this very commit (detect-version in ci.yml) --
462-
# so the subject is the only source available, and the source of truth for
463-
# every other push is the latest release.
464-
- name: Resolve the app release
465-
if: steps.pages.outputs.exists == 'true'
466-
id: release
467-
env:
468-
HEAD_COMMIT_MESSAGE: ${{ github.event.head_commit.message }}
469-
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
470-
run: |
471-
set -euo pipefail
472-
subject=${HEAD_COMMIT_MESSAGE%%$'\n'*}
473-
if [[ "$subject" =~ ^[[:space:]]*(v[0-9]+\.[0-9]+\.[0-9]+): ]]; then
474-
resolved="${BASH_REMATCH[1]}"
475-
echo "Release commit; shipping the chart with ${resolved}."
476-
else
477-
resolved=$(gh api "repos/${GITHUB_REPOSITORY}/releases/latest" --jq .tag_name)
478-
echo "Not a release commit; shipping the chart with the latest release ${resolved}."
479-
fi
480-
if [ -z "$resolved" ]; then
481-
echo "::error::Could not resolve an app release to ship this chart with."
482-
exit 1
483-
fi
484-
echo "version=${resolved}" >> "$GITHUB_OUTPUT"
485-
486478
# Derives appVersion rather than checking it, so the published chart cannot
487479
# be pinned to an older Sim than the release it ships with. The committed
488480
# value is kept current too, but nothing depends on a human remembering.
489481
- name: Sync chart appVersion to the release
490482
if: steps.pages.outputs.exists == 'true'
491483
env:
492-
APP_VERSION: ${{ steps.release.outputs.version }}
484+
APP_VERSION: ${{ needs.release-version.outputs.version }}
493485
run: bun run scripts/sync-chart-appversion.ts --version "${APP_VERSION}"
494486

495487
- name: Configure Git

‎scripts/sync-chart-appversion.ts‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@
2222
* bun run scripts/sync-chart-appversion.ts --version v0.8.26
2323
* bun run scripts/sync-chart-appversion.ts --version v0.8.26 --check
2424
* ```
25+
*
26+
* `--check` verifies both halves: that Chart.yaml names the version and that the
27+
* inventory generated from it is current.
2528
*/
2629
import { spawnSync } from 'node:child_process'
2730
import { readFile, writeFile } from 'node:fs/promises'
@@ -67,7 +70,17 @@ async function main() {
6770
)
6871
process.exit(1)
6972
}
70-
console.log(`appVersion is already ${version}.`)
73+
/**
74+
* The inventory derives from appVersion, so a matching Chart.yaml alone does
75+
* not mean the pair is consistent -- checking only half of what this script
76+
* writes would report success over a stale inventory.
77+
*/
78+
const verified = spawnSync('bun', ['run', 'scripts/generate-image-manifest.ts', '--check'], {
79+
cwd: ROOT,
80+
stdio: 'inherit',
81+
})
82+
if (verified.status !== 0) process.exit(verified.status ?? 1)
83+
console.log(`appVersion is ${version} and the image inventory matches.`)
7184
return
7285
}
7386

0 commit comments

Comments
 (0)