Repository navigation
ci: separate docs build from site publishing - #7386
dajiaohuang wants to merge 1 commit into
Conversation
github-actions
Bot
added
build
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
The permission split puts dependency installation in the read-only job. I found a way for the downloaded site to replace the writer’s Git metadata, even though both new preflight checks pass. The first inline comment has the reproduction and a one-line fix.
| --delete \ | ||
| --exclude '/.git/' \ | ||
| ../docs/build/html/ \ | ||
| ../site-output/ \ |
There was a problem hiding this comment.
Protect .git regardless of its file type. The trailing slash in --exclude '/.git/' only matches a directory. In a disposable checkout, a regular site-output/.git file made this exact rsync command delete the checkout’s nonempty .git directory and replace it with the supplied file. Git then follows that file’s gitdir: pointer, so subsequent Git commands can use configuration supplied by the read-only builder in the job with contents: write. --exclude '/.git' preserved the checkout in the same test. Please add the regular-file case to the site-copy fixture.
| or not uses[1].startswith("actions/download-artifact@")): | ||
| failures.append("the writer may only check out asf-site and download the site artifact") | ||
| if len(re.findall(r"^ run:", deploy, re.M)) != 1: | ||
| failures.append("the writer must have just the static publication script") |
There was a problem hiding this comment.
Check unnamed workflow steps too. Adding a valid - run: bash site-output/build.sh step after the download still makes this check return true. The regex counts run: only when it appears under a step name. Changing the checkout ref from asf-site to main also passes. Can the check inspect every step’s run or uses key and verify the writer’s checkout ref and path? Both mutations make good negative tests.
| touch docs/build/html/.nojekyll | ||
|
|
||
| - name: Upload generated site | ||
| uses: actions/upload-artifact@v7 |
There was a problem hiding this comment.
Use the retrying upload action for this handoff. A transient FinalizeArtifact 403 here drops the site after the five-profile docs build, and the deploy job has nothing to download. The workflow README calls for ./.github/actions/upload-artifact-retry when a later job consumes an artifact. The builder already has the local action checked out, and it accepts include-hidden-files and if-no-files-found.
Which issue does this PR close?
Closes #7320.
Rationale for this change
The docs build installs and runs dependencies in the same job that can push to the repository. A separate writer keeps the repository write token out of the dependency build. This does not make the generated site trustworthy: a compromised builder can still change the output that is published.
What changes are included in this PR?
contents: readand disable checkout credential persistence. Keep the existing source and rendered Mermaid checks before publishing..asf.yamland.nojekyllas an artifact. A fresh job withcontents: writechecks outasf-site, downloads the artifact, and runs only the existing static copy and publication path. The caller's write permission and its main-branch guard remain unchanged..gitduring thersync --delete, explicitly copy.asf.yamlafter rsync, and touch.nojekyll, as the old publication step did. Explicitly copying the metadata matters even when old and new files have the same size and mtime.How are these changes tested?
python3 dev/ci/check-ci-config.pypassed. A separate local mutation check rejected changes to the build permission, persisted credentials, build dependency, hidden-file setting, and deploy download action.python3 dev/ci/test-docs-site-copy.pypassed against synthetic files:.gitwas retained, stale output removed, and.asf.yaml,.nojekyll, and the new page were present. Before restoring the explicit metadata copy, the same-size/same-mtime fixture showed that rsync could retain the old.asf.yaml.actionlint -shellcheck=offandgit diff --checkpassed.make formatandmakepassed on this head in an isolated Linux/JDK 17 environment. The first format attempt failed while downloading an Iceberg dependency (truncated response); one retry succeeded.