ansible / ansible-lint

ansible-lint checks playbooks for practices and behavior that could potentially be improved and can fix some of the most common ones for you
https://ansible.readthedocs.io/projects/lint/
GNU General Public License v3.0
3.49k stars 660 forks source link

single-entry-point: identify role with possible multiple entry points #2242

Open ssbarnea opened 2 years ago

ssbarnea commented 2 years ago

Valid:

Invalid:

Reasonint:

We want to discourage people from writing roles that would be used with tasks_from, mainly because this is fragile and requires extra maintenance, also prone to bugs.

People should use a dispatcher pattern for implementing roles that need multiple kind of actions.

tima commented 2 years ago

This is another controversial rule that got cut from the CoP good practices for good reason. I'm really opposed to forcing it on users by putting it in the production profile similar in #2204. I get wanting to discourage the use of task_from to a degree, but then create a rule that looks for task_from and throw a warning.

I also have an issue with proposing python-isms like the use of a _ prefix. Ansible users and almost always not python programmers. We need to stop letting that stuff bleed through. Also, "all other files are in subdirectories" can be interpreted different ways by different developers and would need to be more specific. Doesn't matter though because this shouldn't be in here.

So I am a big corporation who's developers have written hundreds of playbooks using hundreds of roles that are tested and in production. This tool gets released with this rule in the production profile. Then a senior IT manager overseeing automation tells all their developers "nothing goes into production until it passes Ansible's production profile" as a matter of good practice. Those developers now have to refactor and re-test all of their working roles even though they aren't using task_from. Why? (#2204 has the same effect here.)

It's fine to recommend encourage this pattern, but this isn't the place to do that. Do it in a blog posts or workshops or scaffolding tools.

cognifloyd commented 2 years ago

I use roles with many tasks files to help group the tasks logically. I explicitly use tasks_from as well in many cases where I need a subset of what a common role provides.

kpfleming commented 2 years ago

Same here... this one would go into my skip list very quickly :-)

lucianoventura commented 1 year ago

I see no reason at all for this rule!

And there is no sense for: "this is fragile and requires extra maintenance, also prone to bugs."

"tasks_from" create a powerful way to create good entry points in a role. Any developer can create bugs regardless of this option.

I kindly ask that you review carefully this option.

ssbarnea commented 1 year ago

First, be assured that this rule is far from being our too priority and if it will see the light of days, then will only be as a notification, warning or opt-in.

To understand why is bad, one needs to understand why "goto" was basically banned from most high level languages. It is like starting a program by jumping in the middle if it.

Now, if it was possible to declare (role meta?) which task files are entry points and which one are not, it would be possible to make is less dangerous. Still, this assumes that the role author can do that, which might not be the same person as the role consumer.

kpfleming commented 1 year ago

To some extent that is possible now, by defining the valid entrypoints in meta/argument_specs.yml.

ssbarnea commented 1 year ago

That is indeed great as we did not want to blindly tell everyone to stop using tasks_from. I guess we can consider tasks that is not exposed inside argument_specs as being "private interface", and obviously rename the rule to advise again use of private interfaces.

bluikko commented 1 year ago

It is funny how programming concepts and terms are thrown around here.

And when we hit yet another limitation in Ansible, one of the standard answers is "Ansible is not programming".

Looping blocks? Blocks as handlers? include_role as handler? A look at some of the long, long issues at ansible repository and it is always the same responses: "Ansible is not programming"! "We should not support this"!