Repository navigation
ci: separate docs build from site publishing - #7386
dajiaohuang wants to merge 2 commits 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.
comphead
left a comment
comphead
left a comment
There was a problem hiding this comment.
Thanks for splitting the writer from the dependency build. The /.git exclude, the retrying upload and the extra negative tests from the earlier round look addressed at this head. The comments below are about what the writer still takes from the builder, and where the new checks can be tighter.
| ../site-output/ \ | ||
| ./ | ||
| cp ../.asf.yaml . | ||
| cp ../site-output/.asf.yaml . |
There was a problem hiding this comment.
.asf.yaml now comes from the artifact, so the read-only builder decides what lands on asf-site. Before this change it was copied from the checkout (cp ../.asf.yaml .). The ASF .asf.yaml docs say the file "holds a great deal of power", and on this branch its publish: block (whoami, subdir, hostname) decides where the site is deployed. That is more than the changed pages the description accepts.
Could the writer take .asf.yaml from the commit instead? For example, a second actions/checkout in deploy-docs with sparse-checkout: .asf.yaml, path: trusted and persist-credentials: false, then cp ../trusted/.asf.yaml . after the rsync. The "Prepare site metadata" step, include-hidden-files: true and the matching assertions in check_docs_permission_boundary would then go away, since they exist only to carry .asf.yaml and .nojekyll, which the writer touches itself at line 145. I have not run this variant.
| name: comet-docs-site | ||
| path: docs/build/html/ | ||
| include-hidden-files: true | ||
| if-no-files-found: error |
There was a problem hiding this comment.
This artifact is only consumed by the next job. The other job-to-job handoffs in this repo set retention-days: 1 (pr_build_linux.yml and pr_build_macos.yml for native-lib-*, publish_snapshot.yml for libcomet-linux-*, iceberg_spark_test_reusable.yml for the native lib). Without it the whole built site is kept for the repository default of 90 days. Could this upload set retention-days: 1 as well?
| sudo apt-get install -y -qq --no-install-recommends \ | ||
| libgtk-3-0t64 libnss3 libasound2t64 libgbm1 libatk-bridge2.0-0t64 \ | ||
| libcups2t64 libxkbcommon0 libxdamage1 libxcomposite1 libxrandr2 libxfixes3 | ||
| libcups2t64 libxkbcommon0 libxdamage1 libxcomposite1 libxrandr2 libxfixes3 rsync |
There was a problem hiding this comment.
Is the rsync install needed? The ubuntu-slim readme in actions/runner-images (image 20261026.36.1) lists rsync 3.2.7-1ubuntu1.5 as installed. I have not run a preflight job to confirm. This step is also described as kept identical to the one in docs.yaml, so if the binary does turn out to be missing, a separate install step for the site-copy test would keep the two in sync.
| ("build-docs", build, "read"), | ||
| ("deploy-docs", deploy, "write"), | ||
| ): | ||
| if not re.search(rf"^ permissions:\n contents: {permission}$", block, re.M): |
There was a problem hiding this comment.
This anchors only the first key of the permissions: block, so extra grants pass. I ran check_docs_permission_boundary on a copy of this head with id-token: write added under build-docs after contents: read, and it still returned true. The same holds when the writer's run step executes something from site-output, such as bash ../site-output/x.sh. Could the check require the block to hold exactly one key, and site-output to appear only as the rsync source, with a negative case for each in dev/ci/test-docs-permissions.py?
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.