Skip to content
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

[WIP] Ansible-lint: Add example config for github action #612

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

MarkusTeufelberger
Copy link
Contributor

SUMMARY

resolves #611

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

Tests

ADDITIONAL INFORMATION

Let's see how this works out.

@MarkusTeufelberger
Copy link
Contributor Author

MarkusTeufelberger commented May 31, 2023

Yay, already their documented example is broken:

"Error: Unable to resolve action ansible/ansible-lint@v6, unable to find version v6"

@MarkusTeufelberger
Copy link
Contributor Author

There's also https://github.com/ansible/ansible-lint-action, maybe that's what they meant to use instead? Let's try...

@MarkusTeufelberger
Copy link
Contributor Author

MarkusTeufelberger commented May 31, 2023

Ah well... "CRITICAL Linter finished without analyzing any file, check configuration and arguments given."

Seems like it still has its issues when running against a checked out collection even in a very default layout. Unfortunately the GH-Action seems quite limited when it comes to access to parameter, so setting it to "verbose" to find out what's going on sounds like quite the journey.

@MarkusTeufelberger
Copy link
Contributor Author

I'm actually not sure what it should even lint. We don't provide playbooks or roles outside of tests, so no files to lint seems reasonable. I would expect it to lint galaxy.yml files though, no idea why it doesn't find the one that's right in the root of the repository. Asking in ansible/ansible-lint#3482 what's actually expected to happen and marking this as a draft in the meantime...

@MarkusTeufelberger MarkusTeufelberger changed the title Ansible-lint: Add example config for github action [WIP] Ansible-lint: Add example config for github action May 31, 2023
@felixfontein
Copy link
Contributor

The linter mainly lints the YAML integration tests in tests/integration/targets/. It will produce a HUGE number of errors/warnings/...

My suggestion would be to simply ignore all errors for now.

(But for that it first has to actually run...)

@MarkusTeufelberger
Copy link
Contributor Author

Wow, it is almost impressive how much it complains!

@MarkusTeufelberger
Copy link
Contributor Author

I guess I can add ignores for everything and then start fixing stuff one-by-one either automatically (via ansible-lint --write) or by hand if automatic rewrites are not easy or available. At least the long tail of single digit errors should be doable in this PR I hope.

Signed-off-by: Markus Teufelberger <[email protected]>
.ansible-lint Outdated
@@ -0,0 +1,31 @@
---
skip_list:
# TODO: Fix and enable one-by-one over time
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But only the ones that make sense. sanity[cannot-ignore] looks pretty much wrong, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope that the eventual goal is to have both the collection(s) and ansible-lint rules in a state that no rule skips or any changes to default configs are necessary. For sanity[cannot-ignore] for example I am hardlocked on ansible/ansible#79700 making it into ansible-core and every version of ansible-core falling out of support until I can disable this in my collection. Similar with the shebangs in this repo here apparently?

There are some easy wins with fixing the long tail of errors (end of line whitespace and such) and automatic rewrites could be used or introduced for some things too. There are however also some issues with that (e.g. ansible/ansible-lint#3226 will hit every collection created from the default template).

.ansible-lint Show resolved Hide resolved
@MarkusTeufelberger
Copy link
Contributor Author

I ran against 6.17.0 locally and it complains even more. Added a few quick fixes (on:in github workflow files is a truthy name unfortunately and order apparently is a reserved name by Ansible these days) and 2 more ignores for upcoming rules that were also quite noisy. Now I can also locally start disabling ignores one by one and fix them. First though I also want to get ansible-lint to be idempotent in --write mode (fixing or "fixing" lots of YAML formatting usually). That alone looks like it'll be a horrible thing to review and likely we'll be stuck with an .ansible-lint file in the root directory because it tries to "fix" the github workflow files too.

@MarkusTeufelberger
Copy link
Contributor Author

MarkusTeufelberger commented May 31, 2023

For example

gets butchered HARD and there's no real way not to run YAML re-formatting when using --write mode, but there's also no way that I'm adding hundreds of FQCNs by hand or semi-automated to every task when there's already a perfectly good transform available for that. Unfortunately there is no way currently to only do transforms as ansible-lint kinda depends on being able to completely rewrite everything first.

I guess we can add it as-is and fix a few low hanging fruits over time while keeping the big whoppers around until transforms are available or there is a way that formatting doesn't get completely smashed while keeping liniting available (and non-Ansible files are no longer re-formatted by ansible-lint). I don't really want to start adding tons of config other than rule ignores to the .ansible-lint file, I kinda expect it to work without hassle in a use case like this one (established collection with maybe some issues that wants to keep things from deteriorating and gradually improving).

@MarkusTeufelberger
Copy link
Contributor Author

Looks like they want to use the action from https://github.com/ansible/ansible-lint/blob/main/action.yml actually, not the one from that other repository. I'll update this PR next week thusly.

The other common change in other collections seems to be to use an .ansible-lint-ignore file (https://ansible.readthedocs.io/projects/lint/configuring/#ignoring-rules-for-entire-files) instead of a config file in the root of the repository so new contributions would always have to adhere to all rules instead of allowing "bad" code in and fixing all instances of a rule violation eventually. There doesn't seem to be --write mode applied though.

@felixfontein
Copy link
Contributor

Fine with me, as long as ignore-errors and sanity[cannot-ignore] stay permanently disabled in .ansible-lint .

@MarkusTeufelberger
Copy link
Contributor Author

I'm not sure if it is better to kinda document all 533 rule violations and eventually fix them while not allowing new ones because there's not much to take inspiration from (aka. copy paste) when writing new tests for example. I don't see how current violations are much better or worthy of keeping around though on the other hand it just makes more work for the future to eventually fix all this rule by rule. In any case it is easy enough (and just tedious) to fix stuff, it would just be nice if we could actually use --write mode for this which is currently completely out of question. I now pushed the "minimal rule ignore config + centrally documented ignores + lint config as given in the main repo" version as a comparison.

@github-actions

This comment was marked as off-topic.

@felixfontein
Copy link
Contributor

Also considering that .ansible-lint-ignore is 41 KiB, I think the previous version of the PR was nicer - mainly for the reasons you mentioned :-)

name: Ansible Lint
runs-on: ubuntu-latest
steps:
- name: Run ansible-lint
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A checkout step needs to be added before here. There was a breaking change in the action that started requiring that.

@felixfontein
Copy link
Contributor

@MarkusTeufelberger do you want to further update this? Or should we restart this with a fresh PR? (Or abandon it for now?)

@MarkusTeufelberger
Copy link
Contributor Author

I think I'll rebase and check with the current version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Ansible-lint GitHub action
2 participants