ci: fix ci linter scripts

[tuminoid]

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.

1. Add /usr/src/app to Git safe directories, so Git will run any Git commands, such as "git fetch ..." in that directory.
2. Add "set -e" so command failure will fail the script, to produce correct return code for the check.
3. Due "set -e", and the loop we are running, we need to ignore the linting command returning failure, and capture the failure to be returned later command.
4. We also need to remove the grep from the pipeline. If the commit has no matching files, it would return false error, thanks to "-o pipefail". Same can be applied to all scripts, having Git filter the wanted files, instead of having additional if clause.

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

[netlify]

✅ Deploy Preview for tag-security canceled.

Name	Link
🔨 Latest commit	f2771a5
🔍 Latest deploy log	https://app.netlify.com/sites/tag-security/deploys/65b7fdf77041bc0008339e89

[PushkarJ]

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.

[tuminoid]

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:

• 10000+ markdown complaints
• 10000+ spelling compaints
• 460 dead links

So my original numbers were off 10-fold.

[tuminoid]

tag-lint.txt
tag-spelling.txt
tag-links.txt

[eddie-knight]

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.

[tuminoid]

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.

[PushkarJ]

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!

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.

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 and v1 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.

[tuminoid]

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 and v1 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.

👍

[PushkarJ]

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 🚀

[tuminoid]

Please rebase the branch to current main branch and then we can merge this 🚀

Rebased!