psi4
GHA automate docs build for PRs
Description
This is something I was working on in the spring to find docs problems earlier (currently we'll maybe notice if they break in master) and to remove docs building responsibilities from PR submitters. It looks to still be in working order.
The main contentions are likely to be how to phrase the auto-posts so that PR submitters aren't freaked out and inspired to jumble their git histories beyond repair. Below is a screenshot of what it posts to a PR. I can already see things I'd change. Opening this PR to start discussion on how the auto-post should read.
Status
- [x] Ready for review
- [ ] Ready for merge
I'd prefer to keep the blurb simple, and if at all possible, separate tests from docs. Why not go with something like:
@loriab :
Your PR is modifying Psi4's documentation. To make your life easier, we've built
the updated docs for you. You can preview your changes online at [netlify/...](),
or locally by downloading [this zip archive]().
Please incorporate any further changes by adding commits to this PR.
The samples issue is more complex. I'd avoid trying to teach folks how to use git in an automated blurb.
@loriab:
Your PR is modifying Psi4's test suite. To keep the `samples` suite in sync with
`tests`, we have created an automated commit [checksum]() to your PR branch.
Please review, merge, or otherwise address the changes proposed in the automated commit
into your PR before merging the PR into Psi4's `master`. Feel free to ask for help in
the PR if you have any questions.
I don't understand what Peter means by "separate tests from docs". I suspect the word "tests" should be "samples," and I have no concrete idea as to how this PR combines them. My best guess is have "a build of the documentation target" not be what re-generates samples.
I strongly disagree with removing git command line instructions. They will work in 99% of cases, and not following these instructions could cause merge conflicts that will take more effort from core devs to shepherd new devs through.
For the sample regeneration section: Give examples of why changes might not be okay. Move the bullet point explaining the point of the auto-commit up a level.
For the documentation section: I don't understand the point of the first bullet point. Explain why an edit to this branch might be needed. Explain the difference between deploy-preview and the sphinxman-html download. Make clear that editing the PR branch is also needed for the sphinxman-html download.
Lori's message is for the experienced developer while Peter's is for the beginner. I'd also aim more for the beginner, but include the git commands for the reasons Jonathon mentioned.
- I suspect that what Peter means by separate tests and docs is to issue two comments to the PR? I'm worried ppl will read the last and overlook the former.
- Sounds like I need to make clearer that the only choice for the samples/ commit is now or later. There are no valid reasons to reject, but it you're going to be editing tests a lot, you might not want a dozen automated commits or to have your remote tampered with while local is in a vulnerable state.
- I'm with keeping the git commands as I suspect many ppl are nervous about
--rebaseand--forceand need that reassurance. I do wish I could convey that you're done with local, no need for either path.
Thanks for the comments! I'll do another round of commit-message tweaking. Eventually we can add linters by the same mechanism, so I think it pays to get clear communication and git sync habits established here.
The does-your-PR-break-the-docs and archive-a-tarball-of-your-PRs-docs-for-you-to-check-offline aspects of this PR are long fulfilled. The let-netlify-build-you-a-website-preview-of-your-PRs-docs and have-bot-push-updates-of-samples-to-your-PR aspects are complicated by security and by people having to understand why they can't simple (not force) push to their own PR branches. I don't think these latter aspects are worth the hassle.