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. |
|
Switching PR to draft until all the current releases are out, per the earlier discussion. |
It looks like these releases are all complete. @holdenk - Am I OK to mark this PR as ready for review? To summarize the next steps given what we've discussed so far:
Does this sound good to everyone? |
|
I think we're good we can remove my soft -1 now that the patch releases are done. I do want @celestehorgan to take a look since she ran into some difficulty when she went to update the website. |
celestehorgan
left a comment
There was a problem hiding this comment.
Some non-blocking comments, but thank you for doing this work!
|
I'm ready to move forward with this. And since I can commit to this repo now, I can monitor the deployment and immediately revert this change if anything goes wrong.
|
|
@cloud-fan @holdenk @pan3793 - I'd like the go-ahead from at least one of you before I merge this PR. Do you have any outstanding concerns? |
|
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?
|
Thanks, that answers both questions. I see that dropping the stderr gate is intentional, and that #707 established squash-and-merge for this repository. |
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.