Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add check-spelling #1413

Closed
wants to merge 1 commit into from
Closed

Add check-spelling #1413

wants to merge 1 commit into from

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Apr 7, 2024

This adds https://github.com/marketplace/actions/check-spelling?version=v0.0.22

It's configured to try to use SARIF reporting (which is much prettier than the alternatives). The github/codeql-action kinda sorta needs security-events: write -- which isn't ideal, but eventually it won't and it should only be a concern if you had other workflows that also used security-events (which I don't think you do at this time).

Each of the with items is tunable -- e.g., if you don't want it to check the spelling of file names you can remove that flag.

The advice.md file is included in the GitHub job summary and you can tune it to fit the needs of your repository users.

Often I'll watch a repository for a while to make sure things go smoothly.

If you have questions about things, now or in the future, just tag me. I'm generally fairly responsive.

@kennykerr
Copy link
Collaborator

Sweet, I think I need to enable the action before the workflow will run. I've done that now.

@jsoref
Copy link
Contributor Author

jsoref commented Apr 9, 2024

I've done a git commit --amend --no-edit and git push jsoref HEAD --force-with-lease, but it won't actually do what you're expecting (and I should have been able to tell you that in advance, but I'm still recovering...).

Now is a good time to explain an important detail about this workflow's configuration...

At the end of the day, it will run according to two triggers:

on:
  push:
  pull_request_target:
  • For a push (on:pull), it'll run the version of the workflow being pushed in the repository that's hosting the workflow -- as I just performed a force push to jsoref/cppwinwt, it just triggered https://github.com/jsoref/cppwinrt/actions/runs/8617122327.

    • There's some extra logic where the action itself will look for an open PR for the same branch in the same repository (jsoref/cppwinwt) and if it finds it, it'll skip running a spell check and instead provide a link to where it thinks you can find the pull request report (insert some hand waving: if that hasn't run because of approvals or that event isn't configured).
  • For a pull request (on:pull_request_target), it will run the version of the workflow at the head of the target branch in the target repository. This means that any changes made to the workflow itself will not be reflected in the PR that makes it, but only once it's merged (you can test the changes by applying the file to a fork and then making a PR into the branch in the fork).

    • You will especially notice this behavior when upgrading from one version of the action to the next (but also if you choose to add additional dictionaries or tweak any of the other with: parameters) -- in that the version of the configuration that will be tested in a PR will not be the version listed in the PR -- to test, you'd need an additional PR going into the branch that has the changes (I'd generally suggest a test branch that includes a typo or similar so you can validate the new behavior). For such testing, I tend to use additional forks, but you can, of course, use the main repository.
    • Part of the reason for this behavior is that unlike on:pull_request, workflows with on:pull_request_target are able to have permissions: ...: write which means it wouldn't be safe for GitHub to default to letting a random person make a PR with a workflow with such an event configured to run directly. The decision to accept the workflow is thus managed by the act of merging the workflow at which point it will run (initially via the on:push which happens via the act of merging and then for any future pull requests).

Sorry for the long-winded message and lack of a templated thing. At some point I will probably write this up so I can just either point people to it or copy&paste. But this was the first technical task of my day...

@kennykerr
Copy link
Collaborator

Thanks, this may all be a bit overkill for spell checking but I've certainly learned a lot!

@kennykerr kennykerr closed this Apr 15, 2024
@jsoref jsoref deleted the spell-check branch April 15, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants