-
Notifications
You must be signed in to change notification settings - Fork 950
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
Use --strict to report yaml linting warnings #2295
Conversation
@r-bennett i like the idea :) |
https://github.com/github/super-linter/runs/4718836007?check_suite_focus=true |
I can patch up the warnings on the files - is it possible to run those tests on a branch to check the fixes cover all the cases? |
If you fork this repository and create pull request in your forked repository, you can run CI |
PR which fixes warnings is created: https://github.com/github/super-linter/pull/2301 |
The latest point release includes https://github.com/github/super-linter/pull/2295 which enables a bunch of checks that break existing use.
The latest point release includes https://github.com/github/super-linter/pull/2295 which enables a bunch of checks that break existing use.
The problem with this is that the |
So, from my perspective warnings in yamllint and other linters are supposed to show you that something is not ideal, but not critical enough to fail pipelines. With this change you basically reduced three level quality gates in yamllint to just 2: it's either pass or not. I would say that this should be rolled back or at least add option to disable it. |
It's a yamllint issue: adrienverge/yamllint#430 |
Thanks @ferrarimarco. The specific issue that I mention is indeed adressed there, but @nantiferov may have a valid point about warnings in general. It seems it would be better left to yamllint to decide what should be a warning and what should be an error. At a mimimum, maybe this change should be reverted until the yamllint bug is fixed. As it stands, all GitHub Actions workflow files will fail to lint. |
Hi @adamralph! I think it's worth opening a new issue to discuss this particular yamllint case, and maybe the general rule. IIRC, there may other linters that we invoke in "strict" mode. |
@nantiferov I'm with you on warnings and errors. I don't use the GitHub action so I'm not familiar with how that works, but from my pov you've three things reporting success/warnings/errors:
1 is broken because of adrienverge/yamllint#430 |
This one adrienverge/yamllint#430 is not related to my point at all afaik. Yamllint is working from the beginning in the way that warnings are just shown in output, but not fails pipelines. I have custom yamllint config that put some questionable stuff like line-length to warnings. |
Agreed that adrienverge/yamllint#430 is essentially unrelated, that's just where it fits into the picture and it was mentioned previously.
Disagreed on this - the warnings weren't shown, they were just swallowed. Here's the before and after.
I do also agree with you that the overall behaviour is a bit surprising and sub-optimal, that's why it's probably worth taking this to a fresh issue to discuss as @ferrarimarco has suggested. Afaik this particular change is just bringing yaml behaviour into line with the current behaviour for all other linters |
Ok, probably I saw them when was running locally.
Created a new issue for this https://github.com/github/super-linter/issues/2396 |
The latest point release includes https://github.com/github/super-linter/pull/2295 which enables a bunch of checks that break existing use.
Fixes #
Proposed Changes
Use --strict for yamllint so that warnings are also picked up - as per https://yamllint.readthedocs.io/en/stable/configuration.html#errors-and-warnings we currently get a 0 return code if there are only warnings but no errors, which results in the warnings not being reported. Adding --strict brings the behaviour into line with e.g. xmllint which already returns a non-zero code for warnings only:
Readiness Checklist
Author/Contributor
Reviewing Maintainer
breaking
if this is a large fundamental changeautomation
,bug
,documentation
,enhancement
,infrastructure
, orperformance