oxsecurity / megalinter

🦙 MegaLinter analyzes 50 languages, 22 formats, 21 tooling formats, excessive copy-pastes, spelling mistakes and security issues in your repository sources with a GitHub Action, other CI tools or locally.
https://megalinter.io
GNU Affero General Public License v3.0
1.81k stars 215 forks source link

enhancement: YAMLLINT using --strict causes error and incorrect error count #3612

Closed TommyE123 closed 3 weeks ago

TommyE123 commented 1 month ago

Fixes

When running YAMLLINT with --strict to return non-zero exit code on warnings as well as errors, you will see an error message displayed. This happens when only finding a warning in the file its processed but unable to use the regex to record it. The count is also not reflective of the issues found.

image

Proposed Changes

Regex corrected to also select warnings across all for format options. This code change fixes the error appearing and also counts the warnings when --strict is used and either warnings are found in a file or mixed errors and warnings. Please note with neither --strict or --no-warnings specified, warnings only appear in the list if there’s an error as well. So you won’t see warnings without at least 1 error in a file. If warnings don’t want to be counted and appear in this scenario then use --no-warnings to suppress them.

Test evidence

Github (Tested on Gitlab using --format github --strict) image

Github

image

Parsable

image

Standard

image

Colored

image

Regex Testing

https://regex101.com/r/WaroYf/3

Readiness Checklist

Author/Contributor

Reviewing Maintainer

echoix commented 1 month ago

I remember taking a look at the state of yamllint, see this comment and the ones below (the title of the issue isn't representative of the problem, the errors were shown before the output for a tool) https://github.com/oxsecurity/megalinter/issues/3446#issuecomment-2016934143

So there was more than a way to change the output to make it more parsable.

TommyE123 commented 1 month ago

Apologies, I wasn’t aware that other conversation existed. I now see what you mean!

It seems there can be four possible output formats parsable, standard, standard_color, github depending on the environment where the linter is executed?

I've been testing in an Azure DevOps pipeline. I’m thinking as you mentioned the regex needs to cover other outputs.

Leave this with me and I’ll investigate further and see if there’s anything I can come up with.

Tom

echoix commented 1 month ago

Apologies, I wasn’t aware that other conversation existed. I now see what you mean!

It seems there can be four possible output formats parsable, standard, standard_color, github depending on the environment where the linter is executed?

I've been testing in an Azure DevOps pipeline. I’m thinking as you mentioned the regex needs to cover other outputs.

Leave this with me and I’ll investigate further and see if there’s anything I can come up with.

Tom

No worries, I had to search a little, as it was an ongoing conversation with multiple problems in the same issue.

I think that since we are a tool making the call, we could set the output format to parsable and parse it with that format. If I understand their docs correctly, the goal of this format is to be stable and to be used by tools, just like us. So it's safe to assume that if a user wants to change the output format, it will break the regex count. If the GitHub format isn't too different, it might be fun to have it working too, as it is a format dedicated to be parsable that lets the GitHub interface link errors to the line in the code. If it isn't possible, only keep the parsable format, it's the best bet.

It might be easier to look at their source code (updated), to understand what is what in their format.

TommyE123 commented 3 weeks ago

Hi @echoix,

I've analysed all 4 different format options and updated the regex to handle them all. 😊

When working with log files uploaded to GitHub, I noticed that GitHub parses and formats the output for better display. Therefore, I applied the regex to the behind-the-scenes output for those cases.

Additionally, I've updated the good and bad example test files with the ones I used during testing.

I hope this is now okay.

Best regards, Tom

echoix commented 3 weeks ago

Great! I will take a look later tonight, but great to see you managed to go above and beyond to support all formats!

echoix commented 3 weeks ago

Looks fine! I’ve resolved the merge conflict in the changelog, to allow CI to run one last time.

Thanks again for all the details, screenshots, I was spoiled as a reviewer for this one!

echoix commented 3 weeks ago

Hmm, do you have the same test failures for yaml prettier too? I've already launched a rerun and failed too

TommyE123 commented 3 weeks ago

I've probably broken this by adding my example files at a late stage to the PR and not noticing there were other YAML linters using them to do test fixes.

I can take a look and hopefully sort this.

Tom

echoix commented 3 weeks ago

There are other linters that use specific files just for them, instead of generic ones that apply to other linters of the same language, if you'd rather keep your files and let the original yaml files.

We don't need to test the linter itself, only our integration for it, and that is calling it works correctly + reporting.

TommyE123 commented 3 weeks ago

@echoix,

Hoping this is ok to merge now. 😊

Tom

echoix commented 3 weeks ago

That commit worked great !