Check the roles/plays in this repository with ansible-lint

[MarkusTeufelberger]

Is this a BUG REPORT or FEATURE REQUEST? (choose one):

FEATURE REQUEST

Environment:

Not applicable

Description:

I'd like to propose that besides yamllint, all roles and plays/playbooks in this repository should also be checked with ansible-lint (https://github.com/ansible/ansible-lint).

[Atoms]

we have thought of implementing this, but we have too much to fix for that to work correctly :) maybe in future, or if you want we can look at PR implementing this :)

[MarkusTeufelberger]

I could try to do it check by check (a single PR that fixes everything at once would be horrible to review), it would be nice if someone could help me with adding the initial CI job with all checks disabled to the repository.

[Atoms]

we are in stage to move our CI, so it would be no effort for now to do that as there might be some changes regarding that.

[MarkusTeufelberger]

Do you want to move away from Gitlab CI alltogether or just run your integrations tests elsewhere?

[Atoms]

first we will move tests and already investigating moving away from gitlab

[MarkusTeufelberger]

Hm, do you mean by "it would be no effort for now to do that" that it would be a trivial (effortless) change, or that I should wait a bit until the migration has been done?

[Atoms]

it was meant by now this should not be done :) let's wait migration...

[MarkusTeufelberger]

Great, feel free to ping me once this has been done, then I'll prepare a PR.

I don't think that it would be beneficial to fix linting errors before the linter is available in CI, since it could easily happen that over time people commit code that would fail the linter again.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[Miouge1]

Has been implemented in #4411

[MarkusTeufelberger]

Well, sort of. I'll start with step 2, fixing the actual errors.

[Miouge1]

@MarkusTeufelberger I've started with #4508 #4500 and #4496

Edit: I'm thinking that going 1 error at a time can help to make it faster to review and avoid conflicts

[MarkusTeufelberger]

Great! 👍

[MarkusTeufelberger]

For the record, here's the rough amount of errors when #4411 was merged:

  - '102'  # 1 error
  - '103'  # 2 errors
  - '104'  # 3 errors
  - '201'  # 3 errors
  - '204'  # approx 50 errors
  - '206'  # tons of errors
  - '301'  # approx 50 errors
  - '302'  # 1 error
  - '303'  # 15 errors
  - '305'  # ~30+ errors
  - '306'  # ~40 errors
  - '403'  # 5 errors
  - '404'  # 6 errors
  - '502'  # ~50 errors
  - '503'  # 12 errors
  - '504'  # 3 errors
  - '601'  # ~40 errors
  - '602'  # tons of errors
  - '701'  # 20 errors

[MarkusTeufelberger]

Current state:

• 102 fixed in ansible-lint: when lines should not include Jinja2 variables #4496
• 103 fixed in Fix ansible-lint error 103 #4511
• 104 fixed in ansible-lint: Don't use bare variables [E104] #4608
• 201 fixed in ansible-lint: Add exception for invocation of "rm" [E302, 201] #4609
• 204 ignored in Ignore 2 ansible-lint rules (E204, E701) on purpose. #4744
• 206 fixed in Added space around jinja2 variables #4201/ ansible-lint: add spaces around variables [E206] #4699
• 301 fixed in ???
• 302 fixed in ansible-lint: Add exception for invocation of "rm" [E302, 201] #4609
• 303 fixed in ansible-lint: Fix commands that are also available as module [E303] #4619
• 305 fixed in ???
• 306 fixed in ???
• 403 fixed in ansible-lint: E403 Package installs should not use latest #4500
• 404 fixed in [WIP] ansible-lint: Doesn’t need a relative path in role #4508
• 502 fixed in Added space around jinja2 variables #4201/ Fix Ansible-lint error [E502] #4743
• 503 fixed in ???
• 504 fixed in ansible-lint: Don't use the local_action module [E504] #4666
• 601 fixed in ansible-lint: E601 Don’t compare to literal True/False #4499
• 602 fixed in ansible-lint: don't compare to empty string [E602] #4665
• 701 ignored in Ignore 2 ansible-lint rules (E204, E701) on purpose. #4744

[MarkusTeufelberger]

On the still open ones:

• 204 - good candidate for ignoring (long lines are very prevalent in this repository...)
• 301 - needs expertise about what each command does and how to detect if it already ran in many cases to ensure idempotency
• 305 - good candidate to still be fixed, quite straightforward
• 306 - apparently has some issues on Red Hat derived distros? [306] Shells that use pipes should set the pipefail option - hard to comply on non-RHEL ansible/ansible-lint#497
• 503 - needs expertise to determine if a task really should be a handler that might run much later or if it really needs to run right at this moment
• 701 - good candidate for ignoring (the roles in here are not really designed for galaxy)

[DanyC97]

nice work @MarkusTeufelberger @Miouge1 .

[DanyC97]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[MarkusTeufelberger]

/remove-lifecycle rotten

There is still some work to be done here...

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[Miouge1]

/remove-lifecycle rotten

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[MarkusTeufelberger]

Since some global ignores were even added after I already fixed them(!), I'm no longer going to work on the remaining ones.

Still some work to be done by "someone" though.

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.