Skip to content

Conversation

@hush-hush
Copy link
Member

@hush-hush hush-hush commented Dec 11, 2017

Changes

Starting from ansible 2.4 command line options are now received as a
list. In ansible >=2.4 it's now technically possible to specify multiple
times the '-i' option (even if the documentation say otherwise).

This commit merge all host while removing any extra comas.

This was reported on #23.

@hush-hush hush-hush added the bug label Dec 11, 2017
Copy link
Member

@olivielpeau olivielpeau left a comment

Choose a reason for hiding this comment

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

LGTM! (only one small nit)

Thanks for fixing this 👍


# inventory is a comma separated host list
if isinstance(inventory, list):
inventory = ','.join(inventory)
Copy link
Member

Choose a reason for hiding this comment

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

nit: instead of going from a list to a string (and then back to a list below with inventory.split(',') ; how about normalizing to a listdirectly? i.e.:

if isinstance(inventory, basestring):
    inventory = inventory.split(',')

Or am I missing something? 😃

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually we can have multiple item in the list each containing coma:

ex: -i "web,db" -i "worker,test,admin"
will produce:

inventory = ["web,db", "worker,test,admin"]

So we need to either join the item in the list, or split the item and go over them after to split again.

Copy link
Member

Choose a reason for hiding this comment

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

oh, ok, makes sense then, thanks!

It's a bit counter-intuitive IMO, might be worth adding a comment to explain this behavior :)

Anyway, feel free to merge 👍

Starting from ansible 2.4 command line options are now received as a
list. In ansible >=2.4 it's now technically possible to specify multiple
times the '-i' option (even if the documentation say otherwise).

This commit merge all host while removing any extra comas.
@hush-hush hush-hush force-pushed the maxime/fix-inventory branch from 56ee64c to 862b888 Compare December 12, 2017 16:27
@hush-hush hush-hush merged commit a95b45c into master Dec 12, 2017
@hush-hush hush-hush deleted the maxime/fix-inventory branch December 12, 2017 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants