Fix all yamllint errors and warnings

ryanlerch commented

2026-02-24 11:01:18 +00:00

Owner

fixes all yamllint errors and warnings across the Ansible repository.

Note too, that i changed the CI for yamllint to now run on all files, as there should be no linting errors in the codebase.

fixes all yamllint errors and warnings across the Ansible repository. Note too, that i changed the CI for yamllint to now run on all files, as there should be no linting errors in the codebase.

ryanlerch added 10 commits

2026-02-24 11:01:19 +00:00

Fix yamllint document-start errors by adding YAML document markers 43a5a159f4

Add document start marker (---) to 28 YAML files that were missing it
to comply with yamllint requirements for proper YAML document structure.

Signed-off-by: Ryan Lerch <rlerch@redhat.com>

Fix yamllint trailing-spaces errors by removing trailing whitespace a6f1f6c673

Remove all trailing spaces from YAML files to comply with yamllint
formatting requirements.

Signed-off-by: Ryan Lerch <rlerch@redhat.com>

Fix yamllint octal-value errors by quoting file permission modes 9533c5ad9b

Quote all file permission mode values (0755, 0644, 0600, 0750) as strings
to prevent yamllint from treating them as implicit octal numbers.

Fix yamllint comma spacing errors e866da4e0f

Remove extra spaces before and after commas to comply with yamllint
formatting requirements.

Fix yamllint comment spacing errors e2d9d7f6cb

Add required space after '#' in all comments to comply with yamllint
formatting requirements.

Fix yamllint empty-lines errors ead95bbff8

Remove trailing blank lines and reduce excessive consecutive blank lines
to comply with yamllint formatting requirements.

Fix yamllint new-line-at-end-of-file errors b6a2c9662b

Add missing newline characters at end of files to comply with yamllint
formatting requirements.

Fix yamllint indentation warnings 1745ff420f

Correct indentation to use consistent 2-space indentation throughout
YAML files to comply with yamllint formatting requirements.

Fix yamllint truthy warnings d709e20aaf

- Add .forgejo/workflows/* to ignore list since workflow files use
  GitHub Actions syntax (like 'on:') that doesn't conform to Ansible
  YAML truthy value requirements
- Fix truthy values in vars files to use lowercase 'true'/'false'
  instead of 'True'/'False' to comply with yamllint requirements

Update CI workflow to run yamllint on all files

Linter / yamllint (pull_request) Successful in 30s

Details

Linter / ansible-lint (pull_request) Failing after 31s

Details

1010260b5e

Since all yamllint errors have been fixed, change the workflow to
run yamllint on all YAML files instead of only changed files.
This ensures the entire codebase remains lint-clean.

ryanlerch commented

2026-02-24 11:04:17 +00:00

Author

Owner

Note too the ansible-lint check is likely to fail since this touches so many files, and many still fail if you run ansible-lint on all the files in the ansible repo

ryanlerch force-pushed yamllint-fixes from 1010260b5e

Linter / yamllint (pull_request) Successful in 30s

Details

Linter / ansible-lint (pull_request) Failing after 31s

Details

to 15b84fbd4e

Linter / yamllint (pull_request) Successful in 30s

Details

Linter / ansible-lint (pull_request) Failing after 30s

Details

2026-02-24 11:05:58 +00:00

Compare

kevin commented

2026-02-24 17:44:51 +00:00

Owner

Wow. Thanks for this.

I need to look it over in detail... and it should wait for freeze to be over, but thanks!

Wow. Thanks for this. I need to look it over in detail... and it should wait for freeze to be over, but thanks!

praiskup commented

2026-02-24 19:51:01 +00:00

Member

I have to be honest—I’m not a fan of mass linter fixes. Code should generally only be changed when necessary, and our priority should be to make sure no new warnings are added. Maintaining 'git blame' stability provides much higher value than simply having happy linters. While some might argue that you can't CI-lint a project without 'cleaning house' first, implementing differential scanning is actually quite easy these days (ie, scanning both the base and the proposed code with the same linter version, and only failing if new errors are introduced).

ryanlerch added the

post-freeze

label

2026-02-24 23:32:47 +00:00

kevin commented

2026-03-11 18:28:23 +00:00

Owner

I'm not a super fan of them either, but I think it''s probibly worth doing at this point so we got to a point where people stop ignoring ci checks because they are in things they didn't touch. ;(

But, now this has conflicts... @ryanlerch can you rebase/fix them up? Then we could run the handy ai review on it and see what it says?

I'm not a super fan of them either, but I think it''s probibly worth doing at this point so we got to a point where people stop ignoring ci checks because they are in things they didn't touch. ;( But, now this has conflicts... @ryanlerch can you rebase/fix them up? Then we could run the handy ai review on it and see what it says?

praiskup commented

2026-03-12 07:27:57 +00:00

Member

people stop ignoring ci checks because they are in things they didn't touch.

The problem is that we cannot meet this goal consistently, full-scan linters would anyway bother people in the future. Linter results often change even when the code remains untouched—typically when a new version of yamllint is released or a new policy becomes the default. When this happens, you are forced into another round of mass fixes.

With Adam we previously discussed why differential scanning is the superior approach. Now that we have action runners (moved from Pagure), it seems like low-hanging fruit to implement. I’m happy to contribute the code if the community is interested.

> people stop ignoring ci checks because they are in things they didn't touch. The problem is that we cannot meet this goal consistently, full-scan linters would anyway bother people in the future. Linter results often change even when the code remains untouched—typically when a new version of yamllint is released or a new policy becomes the default. When this happens, you are forced into another round of mass fixes. With Adam we [previously discussed](https://fosstodon.org/@fedoracpt/113094846102751035) why differential scanning is the superior approach. Now that we have action runners (moved from Pagure), it seems like low-hanging fruit to implement. I’m happy to contribute the code if the community is interested.

zlopez commented

2026-03-12 13:14:03 +00:00

Owner

@praiskup This is actually how it's being done. It's only running ansible-lint and yamllint on files that were changed. Unfortunately this is not testing only changes, but whole files. In any case it doesn't run yamllint against the whole repository.

praiskup commented

2026-03-12 16:07:04 +00:00

Member

In any case it doesn't run yamllint against the whole repository.

While this may seem like an advantage, it has a serious consequence: you lose the necessary context for those linters. The only correct way to handle linting is to:

Lock to a specific linter version.
Perform a full scan of the base code.
Perform a full scan of the new proposed code.
Notify the user only about warnings that were not available in previous code.

Analyzing files in isolation provides the worst of both worlds: you lose the necessary context (e.g., code calling "ansible handler" that is nowhere defined), yet you are still burdened with pre-existing issues in that file that you didn't introduce. It is a more awkward compromise than simply performing a scan of a "diff file".

(You may refer to the previous discussion with Adam)

> In any case it doesn't run yamllint against the whole repository. While this may seem like an advantage, it has a serious consequence: you lose the necessary context for those linters. The **only** correct way to handle linting is to: - Lock to a specific linter version. - Perform a full scan of the base code. - Perform a full scan of the new proposed code. - Notify the user only about warnings that were not available in previous code. Analyzing files in isolation provides the worst of both worlds: you lose the necessary context (e.g., code calling "ansible handler" that is nowhere defined), yet you are still burdened with pre-existing issues in that file that you didn't introduce. It is a more awkward compromise than simply performing a scan of a "diff file". (You may refer to the previous discussion with Adam)

adamwill commented

2026-03-12 17:34:37 +00:00

Member

Yeah, I agree with @praiskup 's proposal. For many languages there are tools for doing this - I use diff-cover for Python, for e.g, which also includes a diff-quality implementation which does more or less this with linters (only tells you about new linter errors introduced by the PR). But if something like that doesn't exist for ansible-lint and yamllint, implementing it shouldn't be too hard, if they have useful machine-readable output formats at least.

Yeah, I agree with @praiskup 's proposal. For many languages there are tools for doing this - I use [diff-cover](https://pypi.org/project/diff-cover/) for Python, for e.g, which also includes a `diff-quality` implementation which does more or less this with linters (only tells you about *new* linter errors introduced by the PR). But if something like that doesn't exist for ansible-lint and yamllint, implementing it shouldn't be *too* hard, if they have useful machine-readable output formats at least.

kevin commented

2026-03-12 18:44:09 +00:00

Owner

Yeah, that all makes sense... and I am open to changing to that.

However, we still need to fix things so we have a known good/no errors/warnings state right?

Yeah, that all makes sense... and I am open to changing to that. However, we still need to fix things so we have a known good/no errors/warnings state right?

adamwill commented

2026-03-12 19:10:06 +00:00

Member

No, that's the benefit of the diff approach if you can swing it. It works even if the base state has 50,000 errors; if the checks on the PR show the same 50,000 errors, then it passes. If they show 50,001 errors, it fails. For each PR, the 'base state' is "whatever the linter shows on the branch the PR is against", the 'comparison state' is "whatever the linter shows on the PR branch", and the test passes if there are no new errors. If there are new errors, the test fails and the output is any new errors found.

There are some tricky bits to implementation, though. Say the PR sticks 500 new (perfectly linted) lines into a file which has pre-existing lint errors after those new lines. Now the linter output will be different because the line numbers are different, but it's still the same errors. The implementation needs to be able, somehow, to know that the two outputs are equivalent and call this a pass. It's probably not trivial to solve this (and other related situations). It'd probably be useful to look at how diff-cover does it (and maybe steal it; maybe we can even just extend diff-cover to work with yamllint and ansible-lint?). Maybe the trick is to examine the diff and adjust the line numbers appropriately?

edit: note you can't just look for errors which are on lines in the diff, because it's possible for a change to cause an error on an existing line elsewhere.

No, that's the benefit of the diff approach if you can swing it. It works even if the base state has 50,000 errors; if the checks on the PR show the same 50,000 errors, then it passes. If they show 50,001 errors, it fails. For each PR, the 'base state' is "whatever the linter shows on the branch the PR is against", the 'comparison state' is "whatever the linter shows on the PR branch", and the test passes if there are no new errors. If there are new errors, the test fails and the output is any new errors found. There *are* some tricky bits to implementation, though. Say the PR sticks 500 new (perfectly linted) lines into a file which has pre-existing lint errors after those new lines. Now the linter output will be different because the line numbers are different, but it's still the same errors. The implementation needs to be able, somehow, to know that the two outputs are equivalent and call this a pass. It's probably not trivial to solve this (and other related situations). It'd probably be useful to look at how diff-cover does it (and maybe steal it; maybe we can even just extend diff-cover to *work* with yamllint and ansible-lint?). Maybe the trick is to examine the diff and adjust the line numbers appropriately? edit: note you can't just look for errors which are on lines in the diff, because it's possible for a change to cause an error on an existing line elsewhere.

❤️ 1

kevin commented

2026-03-12 19:36:42 +00:00

Owner

Sure, I get that, but then if you never touch the stuff that fails already it never gets cleaned up?

I'm fine with the flow you are all describing, but I still think it might be valuable to clean up at least easy ones so there's not 50,000 errors in things we don't touch much. Not all the time, but since our linting was utterly broken for a long time (it was returning fine 100% of the time/not actually running the checks)

Sure, I get that, but then if you never touch the stuff that fails already it never gets cleaned up? I'm fine with the flow you are all describing, but I still think it might be valuable to clean up at least easy ones so there's not 50,000 errors in things we don't touch much. Not all the time, but since our linting was utterly broken for a long time (it was returning fine 100% of the time/not actually running the checks)

adamwill commented

2026-03-12 20:01:38 +00:00

Member

Sure, I get that, but then if you never touch the stuff that fails already it never gets cleaned up?

Not through this process, no. The idea is that you do that as and when you want to, and the differential checks prevent backsliding.

> Sure, I get that, but then if you never touch the stuff that fails already it never gets cleaned up? Not through this process, no. The idea is that you do that as and when you want to, and the differential checks prevent backsliding.

ryanlerch commented

2026-03-16 02:45:30 +00:00

Author

Owner

I know this is for the yamllint portion, but AIUI, ansible-lint is a bit harder to effectively run on just changed files or changed parts of files.

If a clean blame is what you are trying to save, would it be better to do all linting changes in a single commit, so you can use --ignore-revs when trying to git blame and running into the linting commit?

I know this is for the yamllint portion, but AIUI, ansible-lint is a bit harder to effectively run on just changed files or changed parts of files. If a clean blame is what you are trying to save, would it be better to do all linting changes in a single commit, so you can use --`ignore-revs` when trying to git blame and running into the linting commit?

praiskup commented

2026-03-16 08:17:05 +00:00

Member

@adamwill

The implementation needs to be able, somehow, to know that the two outputs are equivalent and call this a pass. It's probably not trivial to solve this

Yes, some of those tricky line movements are trickier than others; but in general, csdiff solves this problem a decent way for years (we've been using it for pylint in vcs-diff-lint).

@kevin

Sure, I get that, but then if you never touch the stuff that fails already it never gets cleaned up?

Yeah, but, ... typically we only care about very specific warnings (only those that have real potential to strike, e.g. security related families of errors). The rest is just better to ignore.

I'm fine with the flow you are all describing, but I still think it might be valuable to clean up at least easy ones

This is where I disagree, but it's just an individual's perspective (fixing linter warnings only ever caused hard times to me, never anything good).

@ryanlerch

I know this is for the yamllint portion, but AIUI, ansible-lint is a bit harder to effectively run on just changed files or changed parts of files.

Well, I think we can create a simple hook for ansible-lint into the linter? Off hand, I don't see any reason this should be challenging?

would it be better to do all linting changes in a single commit

Certainly! One commit is much better than a dozen.

@adamwill > The implementation needs to be able, somehow, to know that the two outputs are equivalent and call this a pass. It's probably not trivial to solve this Yes, some of those tricky line movements are trickier than others; but in general, [csdiff](https://github.com/csutils/csdiff) solves this problem a decent way for years (we've been using it for pylint in [vcs-diff-lint](https://src.fedoraproject.org/rpms/vcs-diff-lint)). @kevin > Sure, I get that, but then if you never touch the stuff that fails already it never gets cleaned up? Yeah, but, ... typically we only care about **very specific** warnings (only those that have real potential to strike, e.g. security related families of errors). The rest is just better to ignore. > I'm fine with the flow you are all describing, but I still think it might be valuable to clean up at least easy ones This is where I disagree, but it's just an individual's perspective (fixing linter warnings only ever caused hard times to me, never anything good). @ryanlerch > I know this is for the yamllint portion, but AIUI, ansible-lint is a bit harder to effectively run on just changed files or changed parts of files. Well, I think we can create a simple hook for ansible-lint into the linter? Off hand, I don't see any reason this should be challenging? > would it be better to do all linting changes in a single commit Certainly! One commit is much better than a dozen.

praiskup commented

2026-03-16 08:21:31 +00:00

Member

fixing linter warnings only ever caused hard times to me, never anything good

To clarify, I’m referring to the practice of 'fixing lint just for the sake of having warning-free code.' The reality is that, over the long term, the codebase cleans itself: differential scanning prevents new warnings from being introduced, while legacy warnings naturally disappear as those sections of code are refactored or removed.

> fixing linter warnings only ever caused hard times to me, never anything good To clarify, I’m referring to the practice of 'fixing lint just for the sake of having warning-free code.' The reality is that, over the long term, the codebase cleans itself: differential scanning prevents new warnings from being introduced, while legacy warnings naturally disappear as those sections of code are refactored or removed.

praiskup commented

2026-04-20 22:34:43 +00:00

Member

Just a quick experiment with vcs-diff-lint (csdiff command-line wrapper) and yamllint here:
https://github.com/fedora-copr/vcs-diff-lint/pull/39

If vcs-diff-lint is installed (from the Copr build triggered by Packit), one can run the command against this PR:
vcs-diff-lint --compare-against 5134a6c604e05852e68a07f752d2477d039b7a3e --linter-tag yamllint --print-fixed-errors
(no new yaml errors added, many are fixed)

Just a quick experiment with vcs-diff-lint (csdiff command-line wrapper) and yamllint here: https://github.com/fedora-copr/vcs-diff-lint/pull/39 If vcs-diff-lint is installed (from the Copr build triggered by Packit), one can run the command against this PR: `vcs-diff-lint --compare-against 5134a6c604e05852e68a07f752d2477d039b7a3e --linter-tag yamllint --print-fixed-errors` (no new yaml errors added, many are fixed)

praiskup referenced this pull request from a commit

2026-04-24 10:52:14 +00:00

ci: implement differential yamllint scan

praiskup commented

2026-04-24 10:56:59 +00:00

Member

Take a look at the approach here, feedback is welcome: #3304

Take a look at the approach here, feedback is welcome: https://forge.fedoraproject.org/infra/ansible/pulls/3304

❤️ 1

praiskup referenced this pull request from a commit

2026-04-24 12:56:35 +00:00

praiskup referenced this pull request from a commit

2026-04-29 10:07:42 +00:00

praiskup referenced this pull request from a commit

2026-05-04 19:33:23 +00:00

ci: implement differential yamllint scan

kevin commented

2026-06-01 18:46:32 +00:00

Owner

So, I am fine with the differential scan, thats fine.

I still like the idea of this as a one off thing to fix all the outstanding junk so we start from a good / better baseline.

I'll be happily shouted down if all of you disagree.

So, I am fine with the differential scan, thats fine. I still like the idea of this as a one off thing to fix all the outstanding junk so we start from a good / better baseline. I'll be happily shouted down if all of you disagree.

kevin commented

2026-07-07 21:59:51 +00:00

Owner

related discussion in https://discussion.fedoraproject.org/t/should-we-restore-pre-commit-in-infra-ansible/195507/2

kevin referenced this pull request from a commit

2026-07-07 22:11:15 +00:00

ci: implement differential yamllint scan

kevin referenced this pull request

2026-07-16 17:49:19 +00:00

Add variables to ansible-lint config #3522

jpodivin referenced this pull request

2026-07-20 11:42:46 +00:00

Add variables to ansible-lint config #3522

ryanlerch force-pushed yamllint-fixes from 15b84fbd4e

Linter / yamllint (pull_request) Successful in 30s

Details

Linter / ansible-lint (pull_request) Failing after 30s

Details

to cc1db32428

Linter / yamllint (pull_request) Successful in 36s

Details

Differential yamllint / yamllint-job (pull_request) Successful in 50s

Details

Linter / ansible-lint (pull_request) Failing after 1m20s

Details

2026-08-05 01:57:00 +00:00

Compare

ryanlerch force-pushed yamllint-fixes from cc1db32428

Linter / yamllint (pull_request) Successful in 36s

Details

Differential yamllint / yamllint-job (pull_request) Successful in 50s

Details

Linter / ansible-lint (pull_request) Failing after 1m20s

Details

to 04dbd9cbfe

Linter / yamllint (pull_request) Successful in 36s

Details

Differential yamllint / yamllint-job (pull_request) Successful in 45s

Details

Linter / ansible-lint (pull_request) Failing after 3m11s

Details

AI Code Review / ai-review (pull_request_target) Successful in 30s

Details

2026-08-05 02:02:19 +00:00

Compare

ryanlerch commented

2026-08-05 02:08:15 +00:00

Author

Owner

@kevin have rebased and fixed conflicts.

also fixed a bunch of new warnings that were introduced since this was last rebased.

all but one were in existing categories, so i put the changes into each commit for them.

this was the new change, which should be checked -- !3143 (commit 04dbd9cbfe)

i assume we just want to squash and merge here in forge when we do merge this.

@kevin have rebased and fixed conflicts. also fixed a bunch of new warnings that were introduced since this was last rebased. all but one were in existing categories, so i put the changes into each commit for them. this was the new change, which should be checked -- https://forge.fedoraproject.org/infra/ansible/pulls/3143/commits/04dbd9cbfe8bdf5cf33abcbf9e91bd06234233fc i assume we just want to squash and merge here in forge when we do merge this.

kevin commented

2026-08-05 18:20:42 +00:00

Owner

Ok. Yeah, that change was my bad... I meant to delete the 1 there, but apparently I didn't. ;(

@adamwill @praiskup I'm still inclined to merge this so we start from some kind of 'mostly passing' baseline...

but feel free to talk me out of it?

Ok. Yeah, that change was my bad... I meant to delete the 1 there, but apparently I didn't. ;( @adamwill @praiskup I'm still inclined to merge this so we start from some kind of 'mostly passing' baseline... but feel free to talk me out of it?

adamwill commented

2026-08-05 18:33:27 +00:00

Member

you have successfully poked this ticket enough times that I no longer care and just want the emails to stop

kevin commented

2026-08-05 21:18:55 +00:00

Owner

Sorry. Lets see what the clanker has to say?

kevin added the

ai-review-please

label

2026-08-05 21:19:04 +00:00

forgejo-actions commented

2026-08-05 21:19:35 +00:00

AI Code Review

Detailed Code Review

This is a massive and well-executed cleanup effort. By bringing the entire repository to a zero-warning state, you provide a clear path forward for maintaining linting hygiene.

I have thoroughly reviewed the automated and manual fixes applied throughout the repository. Specifically, quoting octal representations for file permissions (e.g., mode: "0755") is exactly the right approach for Ansible and prevents implicit octal conversion issues in strict YAML parsers. Furthermore, standardizing on lowercase booleans (false instead of False) guarantees compatibility with both YAML 1.1 and 1.2 specifications.

The removal of the duplicate persistentvolumeclaims key correctly retained the parameterized version, which resolves the lint error while preserving the intended dynamic behavior. No regressions, logic errors, or security concerns were identified during this review.

✅ Summary

Overall Assessment: No critical issues identified. The linting fixes were carefully applied and functionally preserve the existing automation behavior.
Minor Suggestions:
- As discussed by the maintainers, consider integrating a differential linter (like diff-cover) in future MRs to keep the CI fast and prevent unrelated linting failures when rulesets are updated.

🤖 AI Code Review | Generated with ai-code-review | Model: gemini-3.1-pro-preview

⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.

## AI Code Review ### Detailed Code Review This is a massive and well-executed cleanup effort. By bringing the entire repository to a zero-warning state, you provide a clear path forward for maintaining linting hygiene. I have thoroughly reviewed the automated and manual fixes applied throughout the repository. Specifically, quoting octal representations for file permissions (e.g., `mode: "0755"`) is exactly the right approach for Ansible and prevents implicit octal conversion issues in strict YAML parsers. Furthermore, standardizing on lowercase booleans (`false` instead of `False`) guarantees compatibility with both YAML 1.1 and 1.2 specifications. The removal of the duplicate `persistentvolumeclaims` key correctly retained the parameterized version, which resolves the lint error while preserving the intended dynamic behavior. No regressions, logic errors, or security concerns were identified during this review. ### ✅ Summary - **Overall Assessment:** No critical issues identified. The linting fixes were carefully applied and functionally preserve the existing automation behavior. - **Minor Suggestions:** - As discussed by the maintainers, consider integrating a differential linter (like diff-cover) in future MRs to keep the CI fast and prevent unrelated linting failures when rulesets are updated. --- 🤖 **AI Code Review** | Generated with [ai-code-review](https://gitlab.com/redhat/edge/ci-cd/ai-code-review) | **Model:** `gemini-3.1-pro-preview` ⚠️ *AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.*

kevin commented

2026-08-05 21:27:19 +00:00

Owner

it loves it! What could possibly go wrong! :)

kevin merged commit a947acc6d3 into main

2026-08-05 21:27:28 +00:00

Rows
Columns

Fix all yamllint errors and warnings #3143

AI Code Review

Detailed Code Review

✅ Summary