comphead commented on code in PR #7386:
URL: https://github.com/apache/datafusion-comet/pull/7386#discussion_r4459281548
##########
.github/workflows/docs.yaml:
##########
@@ -101,17 +99,49 @@ jobs:
# node, and a green build. This runs before the publish step for that
reason.
run: python3 dev/ci/check-mermaid.py --built docs/build/html
+ - name: Prepare site metadata
+ run: |
+ cp .asf.yaml docs/build/html/.asf.yaml
+ touch docs/build/html/.nojekyll
+
+ - name: Upload generated site
+ uses: ./.github/actions/upload-artifact-retry
+ with:
+ name: comet-docs-site
+ path: docs/build/html/
+ include-hidden-files: true
+ if-no-files-found: error
+
+ deploy-docs:
+ name: Deploy docs
+ needs: build-docs
+ runs-on: ubuntu-24.04
+ permissions:
+ contents: write
+ steps:
+ - name: Checkout asf-site branch
+ uses: actions/checkout@v7
+ with:
+ ref: asf-site
+ path: asf-site
+
+ - name: Download generated site
+ uses: actions/download-artifact@v8
+ with:
+ name: comet-docs-site
+ path: site-output
+
- name: Copy & push the generated HTML
run: |
set -x
cd asf-site/
rsync \
-a \
--delete \
- --exclude '/.git/' \
- ../docs/build/html/ \
+ --exclude '/.git' \
+ ../site-output/ \
./
- cp ../.asf.yaml .
+ cp ../site-output/.asf.yaml .
Review 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.
##########
.github/workflows/ci.yml:
##########
@@ -202,7 +202,7 @@ jobs:
sudo apt-get update -qq
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
Review 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.
##########
.github/workflows/docs.yaml:
##########
@@ -101,17 +99,49 @@ jobs:
# node, and a green build. This runs before the publish step for that
reason.
run: python3 dev/ci/check-mermaid.py --built docs/build/html
+ - name: Prepare site metadata
+ run: |
+ cp .asf.yaml docs/build/html/.asf.yaml
+ touch docs/build/html/.nojekyll
+
+ - name: Upload generated site
+ uses: ./.github/actions/upload-artifact-retry
+ with:
+ name: comet-docs-site
+ path: docs/build/html/
+ include-hidden-files: true
+ if-no-files-found: error
Review 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?
##########
dev/ci/check-ci-config.py:
##########
@@ -1928,11 +1928,55 @@ def check_scheduled_workflows_stay_upstream():
return not failures
+def check_docs_permission_boundary():
+ """Keep dependency builds away from the site publishing token."""
+ path = WORKFLOWS / "docs.yaml"
+ jobs = job_blocks(path)
+ build = jobs.get("build-docs", "")
+ deploy = jobs.get("deploy-docs", "")
+ caller = job_blocks(CI_WORKFLOW).get("docs", "")
+ failures = []
+ for label, block, permission in (
+ ("caller", caller, "write"),
+ ("build-docs", build, "read"),
+ ("deploy-docs", deploy, "write"),
+ ):
+ if not re.search(rf"^ permissions:\n contents: {permission}$",
block, re.M):
Review 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`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
