sunchao commented on code in PR #7386:
URL: https://github.com/apache/datafusion-comet/pull/7386#discussion_r4497670628
##########
.github/workflows/docs.yaml:
##########
@@ -101,17 +99,55 @@ 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: Upload generated site
+ uses: ./.github/actions/upload-artifact-retry
+ with:
+ name: comet-docs-site
+ path: docs/build/html/
+ retention-days: 1
+ 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: Checkout trusted site metadata
+ uses: actions/checkout@v7
+ with:
+ ref: ${{ github.sha }}
+ path: trusted
+ sparse-checkout: .asf.yaml
+ persist-credentials: false
+
+ - name: Download generated site
+ uses: actions/download-artifact@v8
Review Comment:
**Retry the download side of the handoff too.** The workflows README
documents intermittent `ListArtifacts` 403s that this action does not retry.
One such response stops publication after the docs build and upload have
succeeded. The writer now checks out the triggering SHA under `trusted`, so it
could include and use the existing `download-artifact-retry` action from that
trusted checkout.
##########
dev/ci/check-ci-config.py:
##########
@@ -1928,11 +1928,64 @@ 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}\n(?= \S|\Z)", block, re.M):
+ failures.append(f"{label} must grant only contents: {permission}")
+ checkout_steps = re.findall(
+ r" - name: [^\n]+\n uses: actions/checkout@[^\n]+(?:\n
[^\n]+|\n with:)*",
+ build,
+ )
+ checkout_count = len(re.findall(r"uses: actions/checkout@", build))
+ if (not checkout_count or len(checkout_steps) != checkout_count
+ or any("persist-credentials: false" not in step for step in
checkout_steps)):
+ failures.append("every build checkout must disable persisted
credentials")
+ if not re.search(r"^ needs: build-docs$", deploy, re.M):
+ failures.append("deploy-docs must depend on the build")
+ steps = re.split(r"^ - ", deploy, flags=re.M)[1:]
+ uses = [re.findall(r"^(?:uses:| uses:) (\S+)", step, re.M) for step
in steps]
+ runs = [re.findall(r"^(?:run:| run:)", step, re.M) for step in
steps]
+ if (len(steps) != 4 or len(uses[0]) != 1 or not
uses[0][0].startswith("actions/checkout@")
+ or len(uses[1]) != 1 or not
uses[1][0].startswith("actions/checkout@")
+ or len(uses[2]) != 1 or not
uses[2][0].startswith("actions/download-artifact@")
+ or uses[3] or runs[0] or runs[1] or runs[2] or len(runs[3]) != 1
+ or not re.search(r"^ ref: asf-site$", steps[0], re.M)
+ or not re.search(r"^ path: asf-site$", steps[0], re.M)
+ or not re.search(r"^ ref: \$\{\{ github.sha \}\}$",
steps[1], re.M)
+ or not re.search(r"^ path: trusted$", steps[1], re.M)
+ or not re.search(r"^ sparse-checkout: \.asf.yaml$",
steps[1], re.M)
+ or not re.search(r"^ persist-credentials: false$",
steps[1], re.M)):
+ failures.append("the writer must check out the site and trusted
metadata before downloading the artifact")
+ if len(steps) == 4:
+ artifact_refs = [line.strip() for line in steps[3].splitlines() if
"site-output" in line]
+ if artifact_refs != ["../site-output/ \\"]:
+ failures.append("the writer may use site-output only as the rsync
source")
Review Comment:
**Check where the artifact lands and what runs after the copy.** I changed
only the download step’s `path` from `site-output` to `.`, and this check still
passed. That would allow archive entries to land in either checkout before the
rsync exclusions apply. In a separate mutation, the checker also accepted `bash
./postprocess.sh` after rsync. At that point the writer is in `asf-site`, where
the artifact controls that file. The current workflow uses the intended path
and commands. Can the guard require the expected download name and destination,
and check the publication commands after the copy? Both mutations would make
useful negative tests.
--
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]
