Closed tuminoid closed 5 months ago
Name | Link |
---|---|
Latest commit | f2771a51635a3345e08ae5140d144a12f23d41c9 |
Latest deploy log | https://app.netlify.com/sites/tag-security/deploys/65b7fdf77041bc0008339e89 |
Thank you so much for opening the issue and PR to fix this.
One request, When you ran this locally can you please paste the output for the jobs in a comment? I want to see if we can fix those spellchecks and lints before we merge this so CI Jobs don't fail on all open PRs.
Thank you so much for opening the issue and PR to fix this.
One request, When you ran this locally can you please paste the output for the jobs in a comment? I want to see if we can fix those spellchecks and lints before we merge this so CI Jobs don't fail on all open PRs.
There is no output on the PR content itself. All the linters check only changed markdown files and this PR does not modify any. It would be the same for all open PRs, they only fail linting on the files they touch.
For local testing, I modified README.md in a way that is invalid for each linter and verified each of the linters caught it.
That said, I do have the logs and will add them. The numbers stand at:
So my original numbers were off 10-fold.
Very excited to see this being caught and remedied.
Is it prudent to use this spellchecker, considering the significant volume of false positives in the sample output? Seems like it'll be more overhead than its worth... This may be a good time to disable that, unless other folks are happy about using it.
Very excited to see this being caught and remedied.
Is it prudent to use this spellchecker, considering the significant volume of false positives in the sample output? Seems like it'll be more overhead than its worth... This may be a good time to disable that, unless other folks are happy about using it.
Having a spell checker is definitely a matter of opinion, especially adding (here, fixing it) to a repository with considerable content violating it.
We (Metal3) also recently started using cspell for our website repository, converting from yaspeller (sending everything to Yandex servers for spellchecking is not cool). It is worthwhile it do spend a bit of time analyzing where the failures are coming from. For example, there is a Spanish document and each word is obviously false negative, so a rule to ignore Spanish documents cuts down the number. Similarly, many hits come from author names, so ignoring a line where authors are listed cuts it more. We also had plenty of issues coming from code fences (triple backticks) and command quote (single backticks), which we now ignore. Combined with a project specific ignore list (you have a list going already), it wasn't too much effort in the end to get to zero spelling issues.
Also, by default, this repository only checks the changed files, so if the PRs are primarily adding new documents, they don't need to worry about cleaning the mess. Only providing clean new documents. This should be easily achievable if few rules are put in place to avoid false positives.
That said, I suggest you merge the fixes to the spell checker (this PR) regardless of the decision. This way you get a working local spell check at least. Then you can disable the spell checking for PRs by removing it from the GH action in separate PR.
Thank you so much again @tuminoid for the PR. I noticed a small edit needed to markdown filtering, otherwise looks good. Let me know if I missed anything!
Hi @PushkarJ ! Thanks for reviewing and testing. You actually caught an issue in the scripts, thanks for that. It is very old fashioned "works on my machine" case, as I have by default enabled shopt -s globstar
on my default shell, and forgot to add it to the linter scripts as well. Globstar option is responsible for expanding **
correctly in Bash, and as it wasn't enabled, it did not pick up any markdown files in subdirectories.
I've fixed the issue in the linter scripts, and personally I'd like to stick with the new way. Mostly as it would be the same in all three scripts and that the possible pipefail in spelling.sh
would mess up the error picking logic. I will of course amend the PR the way you like, if you don't agree, but I'd argue this is cleaner and better way.
Additionally, I agree and confirm that only changed files will get flagged going forward. So this is okay to merge without fixing the whole repo before that.
Just for the record: It has been the case all along, this PR did not chance that.
As an aside, this brings up a good point about translations being the biggest cause of alerts that can be suppressed. We can fix that trivially by moving translations into a separate sub-directory under
v2
andv1
and then ignoring that path here: https://github.com/cncf/tag-security/blob/main/ci/spelling-config.json#L4-L7 . This can definitely be done in a new PR that I hope to get one of our newer contributors to work on.
:+1:
Thank you @tuminoid. I can confirm your fix with globstar option is working so I am okay to keep this.
Please rebase the branch to current main branch and then we can merge this 🚀
Please rebase the branch to current main branch and then we can merge this 🚀
Rebased!
Linting scripts were failing to Git safe dirertory check, both when run via GH actions, or locally via docker-compose. To fix the scripts, few things need to be changed.
Git safe directory check was introduced already in Git 2.35.2 in 2022, and the "ubuntu-latest" image used by the GH action shouldn't have taken too long to upgrade to that or newer Git version. The old GH action logs only go back a month, and they're all showing the same. It could be that the actions have been broken for 1.5 years. The same can bereproduced locally, as the node:18 image used by the docker-compose is also having recent enough Git.
This is tested locally to produce correct behavior for all three cases, and also when no matching files have been changed.
Fixes: #1221