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

Change to FQCN with ansible-lint fixer #553

Merged

Conversation

rholmboe
Copy link
Contributor

@rholmboe rholmboe commented Aug 2, 2024

Proposed Changes

Since ansible-base 2.10 (later ansible-core), FQCN is the new way to go.

Updated .ansible-lint with a production profile and removed fqcn in skip_list. Updated .yamllint with rules needed.

Ran ansible-lint --fix=all, then manually applied some minor changes.

Checklist

  • Tested locally
  • Ran site.yml playbook
  • Ran reset.yml playbook
  • Did not add any unnecessary changes
  • Ran pre-commit install at least once before committing
  • 🚀

@berendt
Copy link
Contributor

berendt commented Aug 2, 2024

I think this only makes sense if a CI test is also added to ensure that automatic formatting is always applied in future. We do this with Black for Python, for example. A test runs in the CI that executes Black and if it makes changes, the test fails.

@rholmboe
Copy link
Contributor Author

rholmboe commented Aug 2, 2024

I think this only makes sense if a CI test is also added to ensure that automatic formatting is always applied in future. We do this with Black for Python, for example. A test runs in the CI that executes Black and if it makes changes, the test fails.

According to already defined CI jobs, it will execute pre-commit run which includes ansible-lint test that will fail if something isn't according to configured standard. So already in place

@timothystewart6
Copy link
Contributor

Hey, thank you! Sounds good! I want to get a few PRs merge in first, which might break your PR but you should be able to fix it easily with ansible-lint --fix=all rather than breaking all of the other current PRs.

@timothystewart6
Copy link
Contributor

@rholmboe sorry about the wait but I wanted merge the other PRs before this since this PR has a lot of style changes.

@rholmboe
Copy link
Contributor Author

rholmboe commented Aug 8, 2024

@rholmboe sorry about the wait but I wanted merge the other PRs before this since this PR has a lot of style changes.

No worries, let me know when I can resolve conflicts and upload a new patch.

@berendt
Copy link
Contributor

berendt commented Aug 8, 2024

@rholmboe sorry about the wait but I wanted merge the other PRs before this since this PR has a lot of style changes.

No worries, let me know when I can resolve conflicts and upload a new patch.

All pending PRs merged. I think you can do a rebase & resolve conflicts.

Since ansible-base 2.10 (later ansible-core), FQCN is the new way to go.

Updated .ansible-lint with a production profile and removed fqcn in skip_list.
Updated .yamllint with rules needed.

Ran ansible-lint --fix=all, then manually applied some minor changes.
@rholmboe rholmboe force-pushed the feature/change-to-fqcn-with-linter branch from 48f5096 to 6ed5868 Compare August 9, 2024 12:23
@rholmboe
Copy link
Contributor Author

rholmboe commented Aug 9, 2024

PR updated, ready for review

@timothystewart6
Copy link
Contributor

@rholmboe it seems like the build is failing.

@rholmboe
Copy link
Contributor Author

@rholmboe it seems like the build is failing.

Yup, applied fix. yamllint failed on octal, something ansible-lint have in ignore.

@timothystewart6
Copy link
Contributor

Thank you!

@timothystewart6 timothystewart6 merged commit b077a49 into techno-tim:master Aug 13, 2024
8 checks passed
@rholmboe rholmboe deleted the feature/change-to-fqcn-with-linter branch August 13, 2024 08:03
@rholmboe
Copy link
Contributor Author

@timothystewart6 no worries man, thank you for creating this and making life easier for the rest of us!

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.

3 participants