ansible-lint
ansible-lint copied to clipboard
ansible
ansible lint should check order of tasks attributes for when and name
Issue Type
- Feature request
Desired Behaviour
Putting name of the task as first attribute should be verified, even if Ansible would allow mentioning name: at any point because it is a dictionary key.
Also when: should be after name: and clearly before the module itself because putting it at the end introduces two problems:
#1 Likely to introduce indentation related bug with blocks, especially when converting a single task to multiple ones inside a block. If when would be before block: this could never happen.
#2 It makes much harder to read tasks, when acts like an if, and is clearly more natural to "if condition do something" and "run something.... if condition", especially as "something" can easily have a big number of lines, maybe not even fitting a single screen.
# good task code:
- name: foo
when: true
module: ...
# bad task code:
- module: ...
name: foo
when: condition
Yes both do execute the same way, but from the maintenance point of view, the first one clearly wins.
I read tasks more like this: "Descriptive title", "the thing that will be done" WHEN "condition is met", so I prefer having the module name as the second member after "name". I'd view your "good" example as bad and would change "module" and "when" around (since "module" will always be there, but "when" is an optional parameter).
I strongly agree that name should always be the first member element.
I agree with the original poster that when should be before the module. This aligns more along with programming languages where the conditional is placed before the executed code. I think using name, when, module helps have the best of both worlds. A description of what should be done, WHEN the condition is met, and how it is accomplished.
@MarkusTeufelberger I can provide a huge number of examples, even from production. Usually when blocks are one line, or at least nor more than 3-4. On the other hand the module parameters can easily be like 10-20 lines (docker ones especially). This means that the when condition could even go outside the screen.
Also, maybe even more important, there are the blocks. If when is after the block, user can easily miss-indent the when and make it part of the last task from the block instead of the entire block. Copy/Paste operations are more likely to cause bugs in those cases.
Also there is the literacy aspect. When works similar to if, and in most (programming) languages you do have `if
Since usage of when should be relatively rare anyways (if it appears often, you're likely trying to "program" in YAML or are better off with including/importing task files instead) and it is an optional parameter, I don't really see the point in having some optional parameters before and some after the module name. Writing when: true everywhere would be a really bad compromise, but also not really. I would even go as far as suggesting that an actual when: true/false line should throw a linter error.
Compared to some other default linter errors/warnings, I would view this more as a stylistic choice that can make sense in some cases and not in others.
By the way:
There's probably a dozen or more possible options for additional task parameters (when, name, module_name, loop, changed_when, failed_when, env, register, no_log...) - do you suggest a canonical order or should when always (if present) be directly after name and directly before module_name (which are probably the only 2 fields that should be present on every task)?
I think I was not clear: when preset, when should be right after name. Obviously that if missing, it should not be added.
Anyway the idea of ordering dictionary items can be extended even further. The easiest way to do it is to use alphabetical order but this is obviously not OK. A good example is name: which we all agree it should be first.
Probably we can make some extensible rules that would help up achieve this. For example, I assume that block should be at the end, in order to avoid the reindentation issues.
I am fully aware that around that area there could be dragons and that it would take time to get it right, and even if when we do, it may still be an optional feature.
One area where ansible's implementation makes this recommendation seem weird is with loops. The when clause is evaluated per item within the loop and therefore the ordering can seem odd. For example:
- name: Check required variables
when: var not in vars
fail:
msg: "Variable '{{ var }}' must be defined."
loop:
- var_1
- var_2
- var_3
loop_control:
loop_var: var
I think at least the name key is without any question (it should always be the first attribute, if present - if not, that's already a different linter error). That by itself would already be a useful rule (in E5xx) - and hopefully not too difficult to write too.
when: true and when: false should probably also be linter errors (in E6xx) that are relatively easy and not contentious (though YAML allows a lot of different ways of expressing boolean values...).
Ordering of optional and/or mandatory keys per task would also be nice to have, but maybe not as errors, but as warnings (so far all rules are errors as far as I understand the naming).
Oh. I like pattern used in https://github.com/psf/black. Any order will be good even one with sorted attributes alphabetically but we definitely needs one.
Also if we introduce this logic we should format as we think will be better. like go fmt terraform fmt yapf, black and so on do.
For attributes to modules, I'd rather go with the one used in documentation than alphabetical...
(1) Should name be first? Yes. The documentation has been clear about that since day one. (I started using Ansible around 1.2, if not earlier.) In fact, name is in some ways kinda special, so it makes sense to think of it as different from the other attributes.
(2) Does the order of any of the other attributes in the dict really matter? No. Many teams already have their own conventions at this point, so it doesn't make a heck of a lot of sense to try to force them to change. Remember, Latin doesn't respect word order either and it wasn't what brought down the Roman / Byzantine Empire.
(3)(a) @MarkusTeufelberger The order used in the modules themselves would be great, but as I know from taking over maintenance of an Atom editor plugin that information isn't exactly readily available to code outside of the Ansible codebase. Sure, you can pull it in, but only in the same way that you can wrap dcraw by calling it from a code library as an external process. Even more annoying, some of it isn't even available in the documentation of the modules themselves as it is part of the documentation of the module they extended (EG the file modules).
(3)(b) As for your comment about using when being a sign of malpractice, you clearly don't have to deal with multiple closely-related and not-so-closely related releases of OS distros from various families all at the same time. For many of us in the world being able to maintain everything on a completely consistent platform that doesn't need conditionals is basically a completely imaginary alternate reality. It doesn't always make sense to break things out into different files the way you state "should" be best, therefore I'm not keen on attempting to force it from inside of a linter.
Well, regarding (3)(b)... I refactored the os-bootstrap role in kubespray (a project that's essentially a bunch of Ansible roles to set up a production ready kubernetes cluster), so I'm more than familiar with dealing with a ton of weirdness (no python available on that distro, no package manager, read-only /usr and /bin, different names for everything...). My point is not that usage of when is a clear anti-pattern, more like a code smell. In some cases it is unavoidable, but in general it makes roles harder to parse as a reader and there are definitely ways to overdo it, as I'm sure you also have seen out there. I am not sure how to translate this into some clear rule though, so I wouldn't suggest any warnings or errors around this. Similar with Jinja code in task files, which might in some cases be the faster and more elegant solution, in others just the "hammer" that someone learned and that now gets applied everywhere something needs to be templated. Also a code smell for me but not something where I would feel confident to be able to write a clear rule for a linter.
A clear first step to resolving this issue by the way would be a rule for ansible-lint that enforces name: to be the first key in every object that contains a key named name. So far after nearly one year I think everyone that commented here clearly agreed on this. Placement of when: and order of other parameters would be different/separate rules anyways I imagine.
We should start doing this only for name for start as it gets lots more complicated after.
@ssbarnea Agreed, start with name. If any others are to be considered they should be settable by local policy, but name should always be first.
I've made a pull request that does this for you - and makes it configurable.
https://github.com/ansible/ansible-lint/pull/2222
@jeefberkey Thanks! I seen your PR and we will discuss it in our team meeting. We want the feature added but we are not yet sure if we want more than one orderlint list.
Another question that we have is where does an unknown property goes? My original vision was that we would have two sorting lists: one for before, and one for after. Any unknown key would end-up in-between these. Obviously that a similar sorting can be achieved if we add a placeholder entry in a single list like * or .*, so it would act as the fallback.
Currently, unknown properties are ignored in the order comparison, because they don't make it into the comparison list. I'll add some docs to my PR shortly.
Can you provide an example of your method? I think I may have actually done it that way, just maybe I didn't expose everything.
Anyway, let me know what the team thinks. I'd love to get this included.
Pretty old subject but... this behavior should be configurable. On our side we prefer to have a direct visibility on the "when" in first place like with real programming langages.
if that:
print("I do this")
- when: that
name: "I do this"
To me Ansible is more like documentation than code (and if you use it like a programming language, you'll have a very painful experience) and thus closer to human language ("I do this when that happens"), but I agree that this is a quite personal style/choice. It definitely can benefit from having a tool that enforces a certain standard either way, I'm not sure if there's a community-wide standard possible though.
Pretty old subject but... this behavior should be configurable. On our side we prefer to have a direct visibility on the "when" in first place like with real programming langages.
if that: print("I do this")- when: that name: "I do this"
Not really, some mistakes from my own point of view:
- you're confusing execution/code with comment/documentation
- if you're considering
nameas a logging instruction, its is printed in all cases even if ignored/skipped. So, procedurale equivalence is:
print("I do this")
if that:
module.exec()
- You keep your procedurale mental model, while most declarative model use condition at end (See SQL)
This was already implemented long time ago and involves more than name and when.