Repository navigation
Automatically build on PRs to the community branch - #4289
Conversation
|
I'm not sure about removing spellcheck.yaml. There's a really easy fix, just change the runner to a current version of Ubuntu, and it will work. Otherwise, if we decide to remove it, it would be better to split the commits. |
02ff8a7 to
1506858
Compare
Also, manually specify list of files to check. Exclude CLAUDE.md because humans aren't editing that file and there's no use in spellchecking it (which is a maintanence burden)
Ok, I put it back. But I still question if it's worth maintaining. As a compromise I excluded CLAUDE.md, because that file is edited by and meant to be read by bots. It could have a lot of technical terminology that doesn't pass the spell checker. (That said I whitelisted it all anyway). |
reusteur73
left a comment
reusteur73
left a comment
There was a problem hiding this comment.
LGTM, approving. The inline comments and the note below are suggestions only and shouldn't block the merge. They can go in a follow-up PR.
Version hash of PR builds
On pull_request events, actions/checkout checks out the temporary merge commit (refs/pull/N/merge), not the head of the PR. As a result, git rev-parse HEAD in the version job returns the hash of that merge commit. From the run on this PR:
HEAD is now at a09a3de5 Merge 0a86768d… into 7e0124e0…
Version: 3.x-dev+a09a3de
a09a3de isn't a commit of the PR (its head is 0a86768) and doesn't exist anywhere else. It also changes every time the base branch moves, even when the PR itself hasn't changed. So a tester reporting /dynmap version gives us a hash we can't trace back.
Building the merge result is still useful, since it checks that the PR compiles against the current base. I'd keep that and only change the hash used for the version:
hash=$(git rev-parse --short=7 "${{ github.event.pull_request.head.sha || 'HEAD' }}")Optionally, we could also add a -prN suffix so a PR jar can't be confused with a branch build.
There was a problem hiding this comment.
I noticed that .github/ISSUE_TEMPLATE/*.md are no longer spellchecked (though we probably won't edit those files much)
There was a problem hiding this comment.
Yeah I tried to include them, but then it reported a huge number of spelling errors. And I still don't want to maintain the spell checking lol. I think they were never scanned at all based on all the errors, ** must not dive into dot directories.
There was a problem hiding this comment.
Since CLAUDE.md is no longer in the spellcheck sources, are these additions still needed? None of these words appear in README.md or CONTRIBUTING.md.
There was a problem hiding this comment.
They are not needed. But maybe they'll show up in the readme someday too ... decided to include them anyway arbitrarily.
|
Decided to apply the github.event.pull_request.head.sha thing now. Missed that earlier. Seems to work, and already picked up the correct hash. |
38a38c6
into
webbukkit:v3.0-community-edition
I think it would be good to verify that PRs sent to the community branch compile, and let us download the actions jars for easier testing. No reason not to, github is more than generous enough to handle it, and in my other main project we set it up this way.
Also, I disabled the spellcheck workflow. It could probably be fixed by updating
runs-on: ubuntu-20.04to a more modern ubuntu but I don't think it's very useful.