Skip to content

fix(ci): publish charts and documentation as one Pages snapshot - #707

Merged
thxCode merged 1 commit into
mainfrom
fix-pages-publication
Sep 30, 2026
Merged

thxCode merged 1 commit into
mainfrom
fix-pages-publication

Conversation

@thxCode

@thxCode thxCode commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator

What type of PR is this?

/kind bug
/area integrations

What this PR does / why we need it:

  • Publish release charts and exact-tag documentation from one Site job, with one Pages checkout, commit and deployment. Keep ordinary Chart CI independent.
  • Package committed chart dependencies without updating them. Preserve existing package bytes on same-source retries and reject moved tags before preparing chart changes.
  • Deploy the actual github-pages content commit with the genuine workflow OIDC token. RC2 publication previously reported deployment success while the public manifest and chart URLs remained stale.
  • Verify public redirect, version metadata, chart index and requested package bytes after deployment. Keep the existing chart index address.
  • Align release guidance with immutable published versions: retry unchanged sources, or choose a new version for a source change. Delete-and-recreate recovery is limited to versions without published chart or site content.

Which issue(s) this PR links to:

Related upstream deployment report: actions/deploy-pages#383

Special notes for your reviewer:

  • make lint and make lint docs passed on the implementation checkout. The isolated submission checkout also passed make lint docs.
  • Node/Python syntax checks and workflow lint passed. The installed actionlint predates the existing concurrency.queue key; only that diagnostic was excluded. The hosted experiments exercised this queue successfully.
  • Local execution of the actual packaging step against RC2 sources and the current Pages snapshot preserved all existing packages and index entries, included the committed bundled charts, preserved bytes on retry, and rejected moved tags and packages without source records before chart changes. No new release tag was created.
  • Hosted runs 36699020529, 36699292363 and 36699425411 deployed two different content commits and replayed unchanged content under the same workflow commit. Public bare URLs matched. Cleanup removed the temporary marker and restored the original content tree.
  • The public verifier passed against the current RC2 manifest, identity files, chart index and package. Its stale-content negative control fails. It samples these publication files rather than every rendered article.
  • Independent review identified the nested-checkout, Helm-version and early-chart-overwrite defects in the first proposal; native packaging and pre-publication validation address them.
  • The complete combined release job still needs a hosted run after merge. Existing RC2 can be retried without moving its tag or rewriting its chart package. Site styling is being prepared separately.

Does this PR introduce a user-facing change?

Publish Helm charts and versioned documentation together, verify public content after deployment, and preserve published chart packages on retries.

Signed-off-by: thxCode <thxcode0824@gmail.com>
@thxCode thxCode added kind/bug Something isn't working area/integrations Kueue, Node Feature Discovery, and the bundled subcharts labels Sep 30, 2026
@gpustack-code-review

gpustack-code-review Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

🔍 OpenCodeReview found 2 issue(s) in this PR.

  • ✅ Successfully posted inline: 0 comment(s)
  • 📋 Routed to summary by policy: 2 comment(s)

maintainability · low

📄 .github/workflows/chart.yml (L36-L37)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category maintainability)

Removing the tags: v* trigger means release tags no longer run any chart lint/generation checks before the Site workflow packages and publishes the chart from the tag's tree. Previously a tag push exercised the lint/install jobs in this workflow. Consider keeping tags: - "v*" in the push trigger (check-only jobs are harmless for tags) or adding a tag-path guard so published charts still pass helm lint — right now a tag whose chart drifted from v*-dev after the last branch CI run publishes unlinted.


bug · low

📄 .github/workflows/site.yml (L112-L113)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category bug)

On a retry of an already-published version, the whole indexing block is skipped. If the existing pages/charts/index.yaml does not already list this version's package (e.g. the package was committed by an earlier partial/failed run that never updated the index, or the index was corrupted/regenerated), the step commits the state as-is: the .tgz is present but never appears in the repo index, and verify.py only compares index bytes against the public site, so the omission is not caught. Consider always ensuring the entry exists on retry, e.g. running helm repo index ... --merge (idempotent for an already-listed version) even when the package file exists, or explicitly asserting the package is listed in index.yaml before skipping.

💡 Suggested Change

Before:

          if [[ ! -f "pages/charts/${package}" ]]; then
            package_dir=$(mktemp -d)

After:

          if [[ ! -f "pages/charts/${package}" ]] || ! grep -q "${package}" pages/charts/index.yaml; then
            package_dir=$(mktemp -d)

@thxCode
thxCode merged commit ef42115 into main Sep 30, 2026
18 checks passed
@thxCode
thxCode deleted the fix-pages-publication branch September 30, 2026 10:35
@thxCode

thxCode commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator Author

Review follow-up after merge: both summary-only findings have been checked.

  • Tag lint/install regression: not confirmed. In the pre-change Chart workflow, both the image and lint-test jobs explicitly skipped tag pushes, and the publisher used linting: off. Removing the obsolete tag publisher did not remove checks that previously ran on tags. Branch CI remains unchanged. Source: pre-change lint-test condition.
  • Existing archive missing from the index: accepted and addressed in follow-up PR fix(ci): validate chart index on publication retries #709, commit e2ef13e. This PR merged before the follow-up could land. The retry now checks the version, package URL filename and SHA-256 digest before committing/deploying Pages. A missing or mismatched entry fails with recovery instructions and preserves the original archive/index. The suggested grep condition was not used because it would repackage a published archive.

Verification for the follow-up: the actual workflow step passed healthy retry and first-publication cases; missing entry, wrong digest, wrong URL filename and wrong version were rejected. Healthy retry preserved the chart tree byte-for-byte, all failure cases left it unchanged, and first publication preserved prior entries and archive bytes. make generate and make lint passed; actionlint passed with only its pre-existing unsupported concurrency.queue key ignored.

The merged main publication run 36703336692 completed successfully, including public-content verification. The published main identity points to merge commit ef42115. The new retry guard itself still requires a hosted release retry after the follow-up merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/integrations Kueue, Node Feature Discovery, and the bundled subcharts kind/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant