Repository navigation
Automatically generate and commit site HTML - #699
Conversation
it's no longer required
|
If you'd like to setup a staging site to test this out, I'm happy to help with that. Otherwise, if/when we merge this, let's confirm the deployed production site is still working as expected so we can revert quickly if necessary. |
|
We're coming up on the 4.3.0 release and I don't want to make @HeartSaVioR's life more difficult, so I don't mind holding off again on moving this forward if he asks for that. At the same time, I really do want to get this done so we can all move on from futzing with generated HTML output. I can just merge this in using my new powers as committer, but as this is a shared workflow for everyone I would prefer to have some formal buy-in on this PR before I do that. @cloud-fan @holdenk @pan3793 - If you could take another look at this PR I would really appreciate it! |
since we want site/ to be purely derivative (as opposed to containing unique source)
it was meant as a convenience but is too risky given how dev scripts on main spark repo are currently written
cloud-fan
left a comment
There was a problem hiding this comment.
Review summary
PR tags: behavior-change
I found two non-blocking inconsistencies in the new contribution guidance and one validation-policy question. The README needs to cover all website source types, and the pull-request template needs to preserve the separately built site/docs/<version> release exception. Please also confirm whether accepting non-fatal Jekyll stderr was an intentional change from the deleted workflow. The automated build-and-publish structure otherwise matches the stated scope, and I found no P0 or P1 blocker, but the unresolved warning-policy choice prevents an unconditional readiness conclusion.
Findings
2 total: 0 P0, 0 P1, 2 P2, 0 P3.
Non-blocking (P2)
- Broaden the source-file guidance —
README.md:9— see inline. - Preserve the release-docs exception —
.github/pull_request_template.md:2— see inline.
Decision challenges
Confirm the Jekyll warning policy
The deleted doc_gen workflow captured Jekyll stderr and failed on messages outside its allowlist, while the shared replacement action only checks the bundle exec jekyll build exit status. Was dropping that warning gate intentional? If not, please preserve equivalent diagnostic filtering in the shared action so both the PR and publication workflows retain it.
|
|
||
| ## Contributing | ||
|
|
||
| To contribute changes, build and test the site locally, then submit a pull request with your changes. You only need to commit changes to the Markdown source. A [GitHub Actions workflow](.github/workflows/html-push.yml) will generate the corresponding HTML under `site/` and push it for you. |
There was a problem hiding this comment.
Non-blocking (P2): js/downloads.js, site/static/versions.json, and other non-Markdown files are website source too, and the release guide asks contributors to commit them. Saying only Markdown needs to be committed can therefore produce incomplete PRs. Please say to commit all source changes while excluding only ordinary Jekyll-generated site output.
There was a problem hiding this comment.
These non-Markdown files are already addressed in the dedicated release guide. The typical PR does not need to worry about them. I'll tweak the wording here without repeating details that IMO are best left to the release guide.
There was a problem hiding this comment.
Thanks, the current wording works for me: it says to commit source files, with Markdown only as the typical case, and leaves release-specific details in the release guide.
| @@ -0,0 +1,5 @@ | |||
| <!-- | |||
| Include your source changes, but not the generated HTML. A GitHub workflow will automatically generate and push the HTML under `site/` in a follow-up commit if necessary. | |||
There was a problem hiding this comment.
Non-blocking (P2): This blanket instruction also covers release PRs, but the release process explicitly commits separately built docs under site/docs/<version>; Jekyll preserves that tree and cannot recreate it. Please exclude only ordinary Jekyll-generated site HTML and call out the versioned release-doc exception.
There was a problem hiding this comment.
Resolved in the current wording: the template excludes only Jekyll-generated HTML and sends release PRs to the guide, which explicitly commits site/docs/ and site/docs/latest.
|
I don't see a merge-method rule established by this PR's code or description. Please use the spark-website maintainer convention for the merge itself; someone with authority over that repository policy should confirm whether squash-and-merge is acceptable. |
|
I reviewed the current changes and have two scoped items: please clarify whether dropping the prior Jekyll stderr/warning gate is intentional, and update the README to say contributors should commit all source changes while omitting only generated site output. |
Yes, intentional. Jekyll reports its exit status correctly, and failing on messages sent on stderr isn't something we do anywhere else. It's not clear to me why this was done in the first place, and the discussion on #316 doesn't add any clarity either.
Hmm, I'm not sure what prompted this comment. But we discussed this on the dev list and I made the corresponding change in #707. All spark-website PRs are henceforth merged only using GitHub's "Squash and merge" button. |
cloud-fan
left a comment
There was a problem hiding this comment.
Review summary
The two documentation inconsistencies from the prior review are resolved in the current README and pull-request template, and the author clarified that relying on Jekyll's exit status is intentional. This round found one late-caught blocker: both replacement jobs dropped the predecessor's pre-checkout disk cleanup, leaving the approximately 23 GiB working tree able to exhaust hosted-runner space before validation or publication starts.
Findings
1 total: 0 P0, 1 P1, 0 P2, 0 P3.
Blocking (P1)
- Restore pre-checkout disk reclamation —
.github/workflows/html-build.yml:13— see inline.
Re-review status
Prior AI findings: 2 addressed, 0 still present; additional unresolved findings in this review: 1.
New attribution: 0 newly introduced, 1 late catch, 0 previously raised, 0 unattributed.
Remaining prior AI findings
No prior AI findings remain.
| name: Build HTML | ||
| runs-on: ubuntu-24.04 | ||
| steps: | ||
| - name: Checkout Spark Website repository |
There was a problem hiding this comment.
Blocking (P1): Both replacement jobs now make checkout their first substantive step, but the deleted job reclaimed large preinstalled packages before checking out this roughly 23 GiB tree. On the selected hosted runner, checkout can exhaust the available disk before either the PR build or the post-merge HTML job reaches the shared action. Please restore sufficient disk preparation ahead of checkout in both workflows, or use an equivalent checkout/storage design with demonstrated capacity.
Verification:
- Inspection: Verify that both affected job definitions place equivalent sufficient disk reclamation ahead of every checkout path.
- Behavior: Verify representative pull-request and non-bot asf-site push executions can complete checkout and reach the shared HTML build on the selected hosted runner.
There was a problem hiding this comment.
The disk cleanup is not necessary. You can see that the build has been running on this PR just fine, and that includes a full checkout of the repo, including site/.
The reason it runs fine is because there is ~90 GB of free space on a 150 GB disk for the default public repo runners. This is different from the 14 GB number shared in the docs.
The nuance here is that the 150 GB disk is an official GitHub commitment for large runners only, not the regular runners we use:
We have no plans to reduce the 150 GB disk on 4-core runners back to 75 GB in the near future. That said, we can't guarantee it will stay that way forever.
So this is working now and will likely work fine for the foreseeable future. That said, if you really want to future-proof this, we can either use a large runner (which I think needs ASF approval) or we can reintroduce some form of disk cleanup step. I personally don't think either is necessary for now, but I'm fine with any approach: a) do nothing; b) use large runner; c) create new composite action for disk cleanup and use it.
@cloud-fan - What would you like to do?
There was a problem hiding this comment.
@cloud-fan - Do you agree we don't need to do anything here, or would you like to reintroduce some kind of disk cleanup?
There was a problem hiding this comment.
Agreed, no change is needed here, and I'm withdrawing this blocker. The old doc_gen cleanup step had been a no-op since #610 removed the job container: it only deleted container tool-cache paths (/__t/...) and ran apt-get remove without sudo behind || true. So the previous workflow already did a full checkout on the same ubuntu-24.04 runner with no effective cleanup, and this PR's Build HTML run confirms the checkout fits. If runner disk ever shrinks, a cleanup composite action can be added then.
|
Thanks, that answers both questions. I see that dropping the stderr gate is intentional, and that #707 established squash-and-merge for this repository. |
cloud-fan
left a comment
There was a problem hiding this comment.
Review summary
This round has no findings, and it withdraws the previous round's only blocker. That P1 asked both replacement workflows to restore the pre-checkout disk cleanup from the deleted doc_gen job. Its premise was wrong. Since #610 removed the job container, that cleanup step had done nothing on the host runner: it deleted container-only tool-cache paths (/__t/CodeQL, /__t/go, /__t/node) that do not exist there, and it ran apt-get remove/autoremove/clean without sudo. Those commands fail for the non-root runner user, and the step still exits 0 because bash runs without -e and the last command is df -h. The baseline already did a full depth-1 checkout on the same ubuntu-24.04 runner with no effective cleanup, so html-build.yml and html-push.yml keep the same checkout capacity. This PR's Build HTML run, which does that full checkout, passed on the current head.
The other concerns raised during this review either follow from the accepted design or are disproved by evidence. Dropping the Jekyll stderr gate is the intentional exit-status-only decision the author explained and the earlier review accepted. Overwriting hand edits to generated site/ files, and the chance of committing local jekyll serve output, both follow directly from the source-only contribution model, and the next [html] regeneration from source corrects them. Using ruby/setup-ruby@v1 is allowed for this repository, as the passing PR build shows. The two documentation items from the first round remain resolved in the current README and pull-request template.
Findings
0 total: 0 P0, 0 P1, 0 P2, 0 P3.
No findings.
Re-review status
Prior AI findings: 1 addressed, 0 still present; additional unresolved findings in this review: 0.
New attribution: 0 newly introduced, 0 late catch, 0 previously raised, 0 unattributed.
Remaining prior AI findings
No prior AI findings remain.
Verification
- The deleted doc_gen 'Free up disk space' step reclaimed no space on the host ubuntu-24.04 runner. It removed only container tool-cache paths under /__t and ran apt-get without sudo, whereas the install step in the same job uses sudo.
- Both replacement workflows run on ubuntu-24.04 and check out at the default depth before invoking the shared build action, the same runner and checkout shape as the deleted job.
- The shared build action checks only the exit status of
bundle exec jekyll build, which matches the author-confirmed decision to drop the stderr gate.
|
I'm away from a computer, but it looks like the website is working fine. I was expecting to merge this in myself so that I could check the site and quickly revert if necessary, but it looks like my hypothesis that the Thank you @cloud-fan and @HyukjinKwon for the review. I will take a closer look at the site to make sure everything is working as expected and also merge in the matching PR on the main repo when I get back to a computer. Feel free to merge that PR in yourselves if you get to it before I do. |
### What changes were proposed in this pull request? Instead of pushing to spark-website twice during a release, push only once. ### Why are the changes needed? apache/spark-website#699 automates the generation of HTML for the website. There could be a race between the release script's two pushes and spark-website's CI push that causes the release script to fail. It wouldn't be a disaster, just annoying. Better to avoid it by pushing to spark-website only once from the release script. Even if apache/spark-website#699 is not merged, this change is still valid (though the comment becomes stale). So it's better to merge this before the spark-website PR is merged. In the future, I think it would be better if we moved the portion of the release script related to the website into the spark-website repo and just called it from here. It's not great that we currently have to open PRs against two separate repos for a change that should in theory be limited to the website repo. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? I'm not sure how to test this holistically. Will think about that. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #59071 from nchammas/website-push. Authored-by: Nicholas Chammas <nicholas.chammas@gmail.com> Signed-off-by: Nicholas Chammas <nicholas.chammas@gmail.com>
### What changes were proposed in this pull request? Instead of pushing to spark-website twice during a release, push only once. ### Why are the changes needed? apache/spark-website#699 automates the generation of HTML for the website. There could be a race between the release script's two pushes and spark-website's CI push that causes the release script to fail. It wouldn't be a disaster, just annoying. Better to avoid it by pushing to spark-website only once from the release script. Even if apache/spark-website#699 is not merged, this change is still valid (though the comment becomes stale). So it's better to merge this before the spark-website PR is merged. In the future, I think it would be better if we moved the portion of the release script related to the website into the spark-website repo and just called it from here. It's not great that we currently have to open PRs against two separate repos for a change that should in theory be limited to the website repo. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? I'm not sure how to test this holistically. Will think about that. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#59071 from nchammas/website-push. Authored-by: Nicholas Chammas <nicholas.chammas@gmail.com> Signed-off-by: Nicholas Chammas <nicholas.chammas@gmail.com>
Site HTML is generated and pushed automatically now (apache#699), so this PR should change contributing.md only.
Add workflows to automatically build the site and push the generated HTML. This eliminates the need for contributors to commit or review HTML changes.
What's Changing
Workflow changes:
html-build.yml: Add a new workflow that runs on PRs and simply generates the docs. This confirms the Jekyll build is not obviously broken.html-push.yml: Add a new workflow that runs on new commits toasf-site. It generates the docs and pushes the generated HTML automatically as a new commit toasf-site.doc_gen.yml: Delete this workflow since it is subsumed by the new workflows.Website source changes:
Repo documentation changes:
committers.mdand the main repo README.content/so its critical role is documented clearly for everyone.This is a second attempt at #697 (which was reverted in #698).
This PR needs to be paired with apache/spark#59071.
Why it's Changing
The current workflow creates a lot of friction. Every website source change generates corresponding HTML output changes. Both need to be committed and part of the PR. This makes the diff messy and difficult to review, especially if the touched source affects multiple pages.
Generating and committing the HTML is also just mechanical grunt work that can and should simply be automated.
Future Work
There are two main improvements we can further make to the workflow here.
master, HTML onasf-site.These improvements are orthogonal to the work in this PR. We can tackle them next.