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

Upgrade check-spelling to v0.0.21 #12249

Merged
merged 25 commits into from
Mar 8, 2023

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Nov 29, 2022

Short description

Upgrades to v0.0.21

https://github.com/check-spelling/check-spelling/security/advisories/GHSA-p8r9-69g4-jwqq

  • Permanently stubs spelling2.yml (as spelling.yml is stubbed)

  • Switches to using sarif reporting using security-events (the model for this is frustrating and almost argues for using a distinct task just as the comment-push job is distinct) for PRs

  • Introduces step summaries (which are longer than comments, but not quite as easy to see)

  • Allows users to talk to the bot in their own forks (it will walk them through the process)

  • candidate.patterns automatically suggest entries for patterns.txt

  • line_forbidden.patterns flags patterns that shouldn't be used

  • act should usually work

  • For more details, see the release notes for v0.0.20 and v0.0.21

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
  • checked that this code was merged to master

@jsoref
Copy link
Contributor Author

jsoref commented Nov 29, 2022

The failed commit 48424f8 has two complaints:

Warning: .github/workflows/spelling3.yml:83:9 ... 21, Warning - use_sarif needs security-events: write - unsupported configuration (unsupported-configuration)
Error: #12249 ... 36, Error - nektos is not a recognized word. (unrecognized-spelling)

nektos

As of 48424f8, check-spelling is configured to check PR titles and descriptions. This can be turned off using:

diff --git a/.github/workflows/spelling3.yml b/.github/workflows/spelling3.yml
index 8a9c6d118..f7c133212 100644
--- a/.github/workflows/spelling3.yml
+++ b/.github/workflows/spelling3.yml
@@ -83,3 +83,2 @@ jobs:
         use_sarif: 1
-        check_commit_messages: title description
         extra_dictionaries:

Or, I could avoid including unrecognized words in the PR title / description. I'm leaning to the former.

sarif

sarif isn't working because it's configured as a pull_request and the rules for PRs from forks: https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token

To limit sarif to the same repository, we could do:

diff --git a/.github/workflows/spelling3.yml b/.github/workflows/spelling3.yml
index f7c133212..e8f0f9a7c 100644
--- a/.github/workflows/spelling3.yml
+++ b/.github/workflows/spelling3.yml
@@ -82,3 +82,3 @@ jobs:
         experimental_apply_changes_via_bot: ${{ github.repository_owner != 'powerdns' && 1 || 0 }}
-        use_sarif: 1
+        use_sarif: ${{ github.event.pull_request.head.repo.full_name == github.repository && 1 }}
         extra_dictionaries:

Note that as-is, it isn't fatal (based on a change I made in my sprint before the release). But, it also isn't particularly helpful.

Alternatively, we could go back to pull_request_target which I believe leads us back to having write permissions.

Note that either way, job step summaries mean users can see a pretty report: https://github.com/PowerDNS/pdns/actions/runs/3574678549#summary-9771582537 -- it just isn't particularly easy to get to it...

.github/workflows/spelling3.yml Outdated Show resolved Hide resolved
.github/workflows/spelling3.yml Show resolved Hide resolved
.github/workflows/spelling3.yml Show resolved Hide resolved
.github/workflows/spelling3.yml Outdated Show resolved Hide resolved
.github/workflows/spelling3.yml Show resolved Hide resolved
.github/workflows/spelling3.yml Show resolved Hide resolved
.github/workflows/spelling3.yml Outdated Show resolved Hide resolved
.github/workflows/spelling3.yml Outdated Show resolved Hide resolved
.github/workflows/spelling3.yml Outdated Show resolved Hide resolved
Copy link
Contributor Author

@jsoref jsoref left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could add comments, or we could try w/o. I'd kinda rather leave them present since it means that the workflow is more likely to work if someone decides to copy+paste it into a private repo (e.g. if there's a security release and someone wants to use CI to make sure things don't break).

.github/workflows/spelling3.yml Show resolved Hide resolved
.github/workflows/spelling3.yml Show resolved Hide resolved
@Habbie Habbie self-requested a review December 6, 2022 18:07
@Habbie Habbie self-assigned this Feb 21, 2023
jsoref and others added 13 commits March 8, 2023 14:32
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
jsoref and others added 11 commits March 8, 2023 14:32
Signed-off-by: Josh Soref <[email protected]>
Using check-spelling/spell-check-this@main
check-spelling/spell-check-this@7adef91

---

Expect isn't being updated by this commit in order to enable the
previous version of check-spelling to report it's happy before it rides
off into the sunset.

The next person to trigger an update to expect will have the chance to
remove the stale items.
As we are no longer commenting, there will not be any confusion
between the two comments, and thus there is no real benefit in
skipping the check run for the push. At most a user will get two
failed runs instead of one.

Signed-off-by: Josh Soref <[email protected]>
At this point, most things do not care.

The main change here will be that spell checking will not check a PR if there are conflicts
@Habbie
Copy link
Member

Habbie commented Mar 8, 2023

I rebased this.

@Habbie Habbie merged commit 2ace8bd into PowerDNS:master Mar 8, 2023
@jsoref jsoref deleted the master-spelling-v0.0.21 branch March 9, 2023 00:29
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