Add ansible-lint and fix some linter warnings

[mboersma]

What this PR does / why we need it:

Adds a make lint target that runs the ansible-lint tool on the playbooks in the images/capi/ansible directory. Also cleans up many simple warnings.

Which issue(s) this PR fixes:

N/A

Additional context

If we think this is a good idea, we can add a test-infra job that runs make lint. There are still 752 warnings after this PR, but it's easy to generate an ignore file. That would allow CI to pass while preventing new linter warnings from creeping in:

ansible-lint ansible/ --generate-ignore

(I didn't add the ignore file to this PR, it's already big enough.)

[mboersma]

% make -C images/capi lint   
hack/ensure-ansible-lint.sh
Collecting ansible-lint==6.19.0
  Obtaining dependency information for ansible-lint==6.19.0

<skipped>

Installing collected packages: ansible-lint
Successfully installed ansible-lint-6.19.0
ansible-lint ansible/
WARNING  Listing 752 violation(s) that are fatal
name[play]: All plays should be named.
ansible/firstboot.yml:15

fqcn[action-core]: Use FQCN for builtin module actions (include_role).
ansible/firstboot.yml:22 Use `ansible.builtin.include_role` or `ansible.legacy.include_role` instead.

name[missing]: All tasks should be named.
ansible/firstboot.yml:22 Task/Handler: include_role name={{ role }}

<skipped>

Read documentation for instructions on how to ignore specific rule violations.

                        Rule Violation Summary                         
 count tag                       profile    rule associated tags       
     6 command-instead-of-module basic      command-shell, idiom       
    11 command-instead-of-shell  basic      command-shell, idiom       
     1 schema[playbook]          basic      core                       
     3 schema[tasks]             basic      core                       
    95 name[missing]             basic      idiom                      
     4 name[play]                basic      idiom                      
     6 yaml[commas]              basic      formatting, yaml           
    14 yaml[line-length]         basic      formatting, yaml           
     1 name[template]            moderate   idiom                      
     6 package-latest            safety     idempotency                
    21 risky-file-permissions    safety     unpredictability           
     4 risky-shell-pipe          safety     command-shell              
     2 ignore-errors             shared     unpredictability           
    52 no-changed-when           shared     command-shell, idempotency 
   421 fqcn[action-core]         production formatting                 
   105 fqcn[action]              production formatting                 

Failed: 752 failure(s), 0 warning(s) on 161 files. Last profile that met the validation criteria was 'min'.
make: *** [lint] Error 2

[AverageMarcus]

/lgtm as best as I can tell

I like the idea of having it as a PR test.

With so many failures currently though we might need to try and get some help to clean things up otherwise I can imagine new PRs containing the same errors due to copying the stuff we already have in place.

I assume the changes in this PR are the result of running it with --fix to auto-fix the easy issues?

I am now wondering if we can get a linter in place for the Makefile to try and get that under control too? 🤔

[AverageMarcus]

/lgtm

(Doesn't look like it liked it in my previous comment)

[jsturtevant]

I like the idea of having it as a PR test.

+1

752 warnings after this PR

Are these things we could open up as a help wanted/good first issue?

/lgtm

[mboersma]

Are these things we could open up as a help wanted/good first issue?

Definitely, most of them are straightforward changes (like always referring to get_url as ansible.builtin.get_url).

[mboersma]

I assume the changes in this PR are the result of running it with --fix to auto-fix the easy issues?

No, I did a bunch of search-and-replace. There isn't a --fix argument unfortunately.

I thought we could merge this, then create a test-infra PR to run make lint. (I thought of piggybacking on make validate-all but that didn't seem appropriate.) Then we can follow up with a PR here to generate an exceptions file so make lint will only fail on newly introduced warnings.

[jsturtevant]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsturtevant

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [jsturtevant]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[AverageMarcus]

This page says there is a --fix flag: https://ansible.readthedocs.io/projects/lint/usage/#using-commands

[mboersma]

Weird, my ansible-lint doesn't seem to have a --fix flag:

% ansible-lint --version
ansible-lint 6.19.0 using ansible-core:2.15.3 ansible-compat:4.1.10 ruamel-yaml:0.17.32 ruamel-yaml-clib:0.2.7
A new release of ansible-lint is available: 6.19.0 → 6.20.0
% ansible-lint --fix images/capi/ansible
usage: ansible-lint [-h] [-P | -L | -T] [-f {brief,full,md,json,codeclimate,quiet,pep8,sarif}] [--sarif-file SARIF_FILE] [-q]
                    [--profile {min,basic,moderate,safety,shared,production}] [-p] [--project-dir PROJECT_DIR] [-r RULESDIR] [-R]
                    [-s] [--write [WRITE_LIST]] [--show-relpath] [-t TAGS] [-v] [-x SKIP_LIST] [--generate-ignore] [-w WARN_LIST]
                    [--enable-list ENABLE_LIST] [--nocolor] [--force-color] [--exclude EXCLUDE_PATHS [EXCLUDE_PATHS ...]]
                    [-c CONFIG_FILE] [-i IGNORE_FILE] [--offline] [--version]
                    [lintables ...]
ansible-lint: error: unrecognized arguments: --fix

[mboersma]

/retest

"failed to create temporary content file: [Errno 104] Connection reset by peer"}
Looks like a flake to me.

[mboersma]

Aha! @AverageMarcus it's because of ansible/ansible-lint#3748

The flag was there already as --write and I missed it. Which is perhaps why they renamed it to --fix in the new version that dropped since I created this PR. :-)

Maybe this will give contributors a head start on fixing things, but I'm sure many of the warnings will require more attention. I'm psyched to get the PR lint job in place and create some help wanted issues.

[mboersma]

/retest

==> Wait completed after 32 minutes 23 seconds

==> Some builds didn't complete successfully and had errors:
--> vsphere-iso.vsphere: Timeout waiting for IP.

==> Builds finished but no artifacts were created.
make: *** [Makefile:455: build-node-ova-vsphere-ubuntu-2004] Error 1

Last time it was flatcar. Seems like the ova build environment is a bit flaky.

I'll run it once more, maybe after that I'll see if /override works here.