Review #549
closed
reformat_yaml_inventory branch
Added by Pierre-Louis Bonicoli almost 8 years ago. Updated almost 8 years ago.
Description
Could you review reformat_yaml_inventory branch ?
Updated by Marc Dequènes almost 8 years ago
- Status changed from New to In Progress
So I'm totally fine with renaming the file, makes it clearer anyway.
As for the content I find it bothersome to begin adding things at a higher level of indentation. Why it is not possible to let them at the previous level?
As for the all group, un ungrouped one was included in all anyway, but it's true it is much better like this. I believe ungrouped is an internal thingy we should not have to care about.
So I wanted to read the commit you're referencing but it's not to be found in the devel branch of Ansible; where may I find it?
Updated by Pierre-Louis Bonicoli almost 8 years ago
Here it is: https://github.com/ansible/ansible/commit/8f97aef1a365cbbbb822d6d09f96af17a076b295.
The ungrouped group is internal to INI plugin inventory (precisely, advanced_host_list and host_list are using it too), there is no such thing with the Yaml plugin. Howewer an ungrouped group containing all hosts can be added. Let me known if I need to update this branch.
Updated by Marc Dequènes almost 8 years ago
Thanks. In fact I found out this commit but with an extra "5" in the commit number (it is truncated in your commit message) and the title does not lead to understand the logic of the YAML inventory as changed. It turns out this change is a "fourre-tout" commit with lots of unrelated changes (typos, unrelated rewording, case change…) so this makes things long and painful to read…
Don't we need to set inventory_enabled? The plugin says To function it requires being whitelisted in configuration. and does not say it is enabled by default (like in the INI plugin).
As for the format, it's true the example shows a tree beginning with the all group, which is not illogical, but really makes reading the file really much less pleasant. The code says otherwise and loops over the dict (see https://github.com/ansible/ansible/commit/8f97aef1a365cbbbb822d6d09f96af17a076b295#diff-72e5b0d4375df72500a9cf3756eba12cR111), so I would be in favor of using the top level hierarchy.
Updated by Pierre-Louis Bonicoli almost 8 years ago
Marc Dequènes wrote:
Don't we need to set inventory_enabled? The plugin says To function it requires being whitelisted in configuration. and does not say it is enabled by default (like in the INI plugin).
The YAML plugin is enabled by default.
As for the format, it's true the example shows a tree beginning with the all group, which is not illogical, but really makes reading the file really much less pleasant. The code says otherwise and loops over the dict (see https://github.com/ansible/ansible/commit/8f97aef1a365cbbbb822d6d09f96af17a076b295#diff-72e5b0d4375df72500a9cf3756eba12cR111), so I would be in favor of using the top level hierarchy.
I don't understand your proposal, could you give an example of the proposed update ? The _parse_group method is used foreach entry of the dict, each entry will define a group. The documentation says YAML based inventory, starts with the 'all' group and has hosts/vars/children entries..
Updated by Marc Dequènes almost 8 years ago
That's what I'm referring too. You're following the doc, which is legitimate, and I wish to not do any change except renaming ungrouped into all because I really dislike this and the code says it is totally ok; the code loops on the top dict, not just on the all declared children.
Updated by Pierre-Louis Bonicoli almost 8 years ago
So I will create first an issue on ansible bugtracker asking for clarification about the all group requirement.
Updated by Pierre-Louis Bonicoli almost 8 years ago
Upstream said:
Pilou | bcoca: with YAML inventory plugin, is the 'all' group mandatory ? bcoca | no, but i recommend it bcoca | will represent actual hierchy that inventory becomes bcoca | also, only way to set 'all:vars'
So I don't have any preference, do you still prefer using ungrouped at the beginning of the inventory ?
Updated by Marc Dequènes almost 8 years ago
- Status changed from In Progress to Resolved
I did not say I do not want a all group, just I dislike having all the other groups under it leading to the whole file indentation shifted twice because of it. It's logical of course, but impact readability. This use of the YAML structure would make sense if we could combine alternate inventories in a single file, which is not possible.
Anyway, thanks for asking for clarification, it is still useful. Let's do this tree as upstream wants it to… to avoid falling into unexpected behaviors in the future.
Thanks for catching dashes in the group names.
Merged.