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

Add ability to load .config/ansible-lint.yml #1822

Merged
merged 1 commit into from
Mar 5, 2022

Conversation

ssbarnea
Copy link
Member

@ssbarnea ssbarnea commented Jan 21, 2022

This will allow users to declutter their project root directory by allowing users to put configuration in .config/ansible-lint.yml instead of .ansible-lint file.

It matches a feature already implemented by molecule and supported by https://dot-config.github.io -- the goal being to reduce cluttering of root project directories.

This does not change the ability to load the file from its current location.

@ssbarnea ssbarnea requested a review from tadeboro January 21, 2022 12:02
@ssbarnea ssbarnea marked this pull request as ready for review January 21, 2022 12:03
@ssbarnea ssbarnea requested a review from a team as a code owner January 21, 2022 12:03
@ssbarnea ssbarnea requested review from relrod, cidrblock and priyamsahoo and removed request for a team January 21, 2022 12:03
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

I've never seen other projects using a leading dot when the config is in a subdirectory. In that case, it's usually only the directory itself that is dotted.
I'd go for .config/ansible-lint, then. Alternatively, there could be support for specifying a different path via pyproject.toml instead of hardcoding one string — because it's not a widely accepted standard and some projects may already have some other directory for config adopted.
Also, I would expect tests for this kind of patch.

@ssbarnea
Copy link
Member Author

I've never seen other projects using a leading dot when the config is in a subdirectory. In that case, it's usually only the directory itself that is dotted. I'd go for .config/ansible-lint, then. Alternatively, there could be support for specifying a different path via pyproject.toml instead of hardcoding one string — because it's not a widely accepted standard and some projects may already have some other directory for config adopted. Also, I would expect tests for this kind of patch.

I am not going to go for the configure the config file location rabbit hole.

I considered a different name without the dot but that would add extra complexity and I prefer to KISS. Telling people that just move existing config filename without asking them to rename them is easier. Also, changing the filename would require changes in other places like json schemas that detect the configuration of ansible-lint.

But I will add tests, that is indeed a genuine comment. Anything else should be considered as something for the future, like having a proper yml file extension would not be a bad idea, just outside the scope.

@tadeboro
Copy link
Contributor

I think this is not needed. Ansible Lint already consults too many locations when it searches for the config file. It is bad enough that the configuration loader can retrieve configuration from a file that has nothing to do with the current project.

As for configuring the config file path: we already allow this (-c command-line switch). So adding support for this would probably be a lesser evil compared to another magic search we are trying to introduce here.

@webknjaz
Copy link
Member

But I will add tests, that is indeed a genuine comment.

All of my comments are genuine. I don't post things I don't mean just because I'm bored. Although, feel free to ask for more reviews to see if somebody else feels file your approach is justified.

@ssbarnea ssbarnea added this to the 5.4.0 milestone Jan 25, 2022
@ssbarnea ssbarnea modified the milestones: 5.4.0, 6.0.0 Feb 12, 2022
@ssbarnea ssbarnea changed the title Add ability to load .config/.ansible-lint Add ability to load .config/ansible-lint.yml Feb 16, 2022
@ssbarnea ssbarnea force-pushed the fix/dot-config branch 2 times, most recently from ad03e96 to b3529d8 Compare February 16, 2022 08:45
@ssbarnea ssbarnea dismissed webknjaz’s stale review February 25, 2022 18:38

Original comments no longer apply to current proposed change.

@ssbarnea ssbarnea merged commit 1013149 into ansible:main Mar 5, 2022
@ssbarnea ssbarnea deleted the fix/dot-config branch March 5, 2022 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants