https://projects.duckcorp.org/https://projects.duckcorp.org/favicon.ico?16699090422017-06-04T01:14:51ZDuckCorp ProjectsDuckCorp Infrastructure - Review #549: reformat_yaml_inventory branchhttps://projects.duckcorp.org/issues/549?journal_id=11972017-06-04T01:14:51ZMarc Dequènesduck@duckcorp.org
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>In Progress</i></li></ul><p>So I'm totally fine with renaming the file, makes it clearer anyway.</p>
<p>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?</p>
<p>As for the <em>all</em> group, un <em>ungrouped</em> one was included in <em>all</em> anyway, but it's true it is much better like this. I believe <em>ungrouped</em> is an internal thingy we should not have to care about.</p>
<p>So I wanted to read the commit you're referencing but it's not to be found in the <em>devel</em> branch of Ansible; where may I find it?</p> DuckCorp Infrastructure - Review #549: reformat_yaml_inventory branchhttps://projects.duckcorp.org/issues/549?journal_id=12052017-06-04T16:03:18ZPierre-Louis Bonicolipierre-louis.bonicoli@ir5.eu
<ul></ul><p>Here it is: <a class="external" href="https://github.com/ansible/ansible/commit/8f97aef1a365cbbbb822d6d09f96af17a076b295">https://github.com/ansible/ansible/commit/8f97aef1a365cbbbb822d6d09f96af17a076b295</a>.</p>
<p>The <code>ungrouped</code> group is internal to INI plugin inventory (precisely, <code>advanced_host_list</code> and <code>host_list</code> are using it too), there is no such thing with the Yaml plugin. Howewer an <code>ungrouped</code> group containing all hosts can be added. Let me known if I need to update this branch.</p> DuckCorp Infrastructure - Review #549: reformat_yaml_inventory branchhttps://projects.duckcorp.org/issues/549?journal_id=12072017-06-04T23:57:44ZMarc Dequènesduck@duckcorp.org
<ul></ul><p>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…</p>
<p>Don't we need to set <em>inventory_enabled</em>? The plugin says <em>To function it requires being whitelisted in configuration.</em> and does not say it is enabled by default (like in the INI plugin).</p>
<p>As for the format, it's true the example shows a tree beginning with the <em>all</em> 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 <a class="external" href="https://github.com/ansible/ansible/commit/8f97aef1a365cbbbb822d6d09f96af17a076b295#diff-72e5b0d4375df72500a9cf3756eba12cR111">https://github.com/ansible/ansible/commit/8f97aef1a365cbbbb822d6d09f96af17a076b295#diff-72e5b0d4375df72500a9cf3756eba12cR111</a>), so I would be in favor of using the top level hierarchy.</p> DuckCorp Infrastructure - Review #549: reformat_yaml_inventory branchhttps://projects.duckcorp.org/issues/549?journal_id=12082017-06-05T12:49:22ZPierre-Louis Bonicolipierre-louis.bonicoli@ir5.eu
<ul></ul><p>Marc Dequènes wrote:</p>
<blockquote>
<p>Don't we need to set <em>inventory_enabled</em>? The plugin says <em>To function it requires being whitelisted in configuration.</em> and does not say it is enabled by default (like in the INI plugin).</p>
</blockquote>
<p>The YAML plugin is enabled by <a href="https://github.com/ansible/ansible/blob/8f7c8ef3a5f49ae7e625a5ac8c793619666974d2/lib/ansible/constants.py#L244" class="external">default</a>.</p>
<blockquote>
<p>As for the format, it's true the example shows a tree beginning with the <em>all</em> 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 <a class="external" href="https://github.com/ansible/ansible/commit/8f97aef1a365cbbbb822d6d09f96af17a076b295#diff-72e5b0d4375df72500a9cf3756eba12cR111">https://github.com/ansible/ansible/commit/8f97aef1a365cbbbb822d6d09f96af17a076b295#diff-72e5b0d4375df72500a9cf3756eba12cR111</a>), so I would be in favor of using the top level hierarchy.</p>
</blockquote>
<p>I don't understand your proposal, could you give an example of the proposed update ? The <code>_parse_group</code> method is used foreach entry of the dict, each entry will define a group. The documentation says <a href="https://github.com/ansible/ansible/blob/5553b208281d723ead1e61be5c37227ec2b2736e/lib/ansible/plugins/inventory/yaml.py#L24" class="external">YAML based inventory, starts with the 'all' group and has hosts/vars/children entries.</a>.</p> DuckCorp Infrastructure - Review #549: reformat_yaml_inventory branchhttps://projects.duckcorp.org/issues/549?journal_id=12092017-06-05T17:34:03ZMarc Dequènesduck@duckcorp.org
<ul></ul><p>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 <em>ungrouped</em> into <em>all</em> because I really dislike this and the code says it is totally ok; the code loops on the top dict, not just on the <em>all</em> declared children.</p> DuckCorp Infrastructure - Review #549: reformat_yaml_inventory branchhttps://projects.duckcorp.org/issues/549?journal_id=12102017-06-06T16:14:04ZPierre-Louis Bonicolipierre-louis.bonicoli@ir5.eu
<ul></ul><p>So I will create first an issue on ansible bugtracker asking for clarification about the <code>all</code> group requirement.</p> DuckCorp Infrastructure - Review #549: reformat_yaml_inventory branchhttps://projects.duckcorp.org/issues/549?journal_id=12122017-06-06T21:24:03ZPierre-Louis Bonicolipierre-louis.bonicoli@ir5.eu
<ul></ul><p>Upstream said:</p>
<pre>
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'
</pre>
<p>So I don't have any preference, do you still prefer using <code>ungrouped</code> at the beginning of the inventory ?</p> DuckCorp Infrastructure - Review #549: reformat_yaml_inventory branchhttps://projects.duckcorp.org/issues/549?journal_id=12142017-06-09T16:44:59ZMarc Dequènesduck@duckcorp.org
<ul><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Resolved</i></li></ul><p>I did not say I do not want a <em>all</em> 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.</p>
<p>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.</p>
<p>Thanks for catching dashes in the group names.</p>
<p>Merged.</p>