Should we restore pre-commit in infra/ansible?

Project Discussion
1

The infra/ansible repository has a configuration file for pre-commit, but with the exception of yamllint, all checks are commented out.

The repository already contains a substantial amount of code that isn’t compliant with best Ansible practices, and this makes introduction of new issues even easier.

At the same time. The yamllint is not pinned to hash, but to tag. Which is not necessarily a big deal. Unless the maintainer were to be compromised, and tag moved so it would point to code that would steal your credentials etc.

I suggest that we restore the hooks and pin them to hashes. The change will impact everyone working in the repo. So I want to bring it up here, before I make a PR.

I know that ansible-lint can be uncompromising, so I propose to create a .ansible-lint-ignore setting the current state of the sources as a baseline.
This way we can avoid making massive changes to the repo to satisfy the linter, and instead improve the quality gradually.

2

The infra/ansible repository has a configuration file for pre-commit, but with the exception of yamllint, all checks are commented out.

yeah.

The repository already contains a substantial amount of code that isn’t compliant with best Ansible practices, and this makes introduction of new issues even easier.

Yeah, but also the ansible-lint stuff was broken and reporting all well
for a long time, so people got used to it, then it was fixed and they
got used to ignoring it. ;(

At the same time. The yamllint is not pinned to hash, but to tag. Which is not necessarily a big deal. Unless the maintainer were to be compromised, and tag moved so it would point to code that would steal your credentials etc.

I suggest that we restore the hooks and pin them to hashes. The change will impact everyone working in the repo. So I want to bring it up here, before I make a PR.

Pinning to hash is reasonable.
Re-enabling the other checks is… possible, but I kinda hate to do that
with a bunch of things broken. Sure, you can fix things in each pr, but
it’s very discouraging to make a small change and have to fix a ton of
things in other places you didn’t want to touch.

We had some discussion about this in:

I still think it’s worth doing a large cleanup so moving forward things
are easier, but others disagreed. :frowning:

I know that ansible-lint can be uncompromising, so I propose to create a .ansible-lint-ignore setting the current state of the sources as a baseline.
This way we can avoid making massive changes to the repo to satisfy the linter, and instead improve the quality gradually.

Yes, we definitely will need to ignore some things.
I thought we already had such a list? Perhaps it was in the zuul job?
ah, no, it’s on the top level, along with yamllint:

Anyhow, I’m in favor of more checks, but I’d personally like to have
things more cleaned up, but in any case we can enable things…

So, from my view a PR is fine. :wink:

3

I’m for cleaning things up as well and we can just update the ansible-lint config to ignore some of the errors that we don’t really care about.

The gradual update sounds nice, but as there are plenty of people working on ansible it would be annoying for them to fix all the alerts just for their small change that they did.

4

The .ansible-lint-ignore is actually a separate file and setting that we don’t have yet. It would allow us to block individual rules on file level. The existing .ansible-lint could also use expansion, specifically the mock_modules section is probably in need of expansion.

5

The .ansible-lint-ignore is actually a separate file and setting that we don’t have yet.

Oh, is that leftover from the zuul setup before?
If so, we should merge all those into whatever is the right place.

It would allow us to block individual rules on file level. The existing .ansible-lint could also use expansion, specifically the mock_modules section is probably in need of expansion.

Configuration - Ansible Lint Documentation

yeah.

6

I’ve made a PR for this, if anyone wants to give it a look. Making sure you're not a bot!

7

Looks reasonable, merged. :slight_smile: We will see if it needs adjustments…