Skip to content

Conversation

@gtmanfred
Copy link
Contributor

What does this PR do?

Ansible now provides an ansible-inventory command which renders the inventory script/file. Because of this, we can now use that command to render the inventory for ourselves, instead of having to maintain code to process the file as well.

Tests written?

Yes

Commits signed with GPG?

Yes

@gtmanfred gtmanfred requested a review from a team as a code owner July 11, 2018 23:39
@ghost ghost requested review from a team July 11, 2018 23:39
Copy link
Contributor

@isbm isbm Jul 12, 2018

Choose a reason for hiding this comment

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

I am really wondering why we would execute that in so heavy-weight fashion spinning off another Python interpreter etc, instead of just reuse the ansible.cli.InventoryCLI for this. As a conceptual example:

try:
    import ansible.cli.inventory as ansible_inventory
except
    ansible_inventory = None

def __virtual__():
    return ansible_inventory and __virtualname__, 'Ansible is not installed on your system'

def ...:

    c = ansible_inventory.InventoryCLI(['', '-i', get_roster_file(__opts__), '--list'])
    c.parse()
    inventory = c.run()

    ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still support python 3.4, which ansible does not, so in order to use ansible-inventory on centos 7, right now you have to pip2 install ansible, and then run the binary.

Until we drop python3.4 support i think this is the better setup.

Copy link
Contributor

@isbm isbm Jul 12, 2018

Choose a reason for hiding this comment

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

Ah, you're right. CentOS 7 and family on 3.4. OK, in this case we should do the fallback then:

return (ansible_inventory or which('ansible-inventory')) and __virtualname__, '....'

I still don't want to spin-off another Python process on an expensive IBM Mainframe, where every bit of RAM is for gold. 😉 Once we stop 3.4 support, we can just drop this part.

Copy link
Contributor Author

@gtmanfred gtmanfred Jul 12, 2018

Choose a reason for hiding this comment

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

.tox/py36/lib/python3.6/site-packages/ansible/cli/inventory.py:186: in run
    exit(0)
_ _ _ _ _ _ _ _

This is why we cannot use the ansible/cli/inventory.py script, because it calls a system exit. and just displays the results instead of returning them.

Copy link
Contributor

@terminalmage terminalmage left a comment

Choose a reason for hiding this comment

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

blind-review

@rallytime rallytime merged commit bdeeed5 into saltstack:develop Jul 16, 2018
@gtmanfred gtmanfred deleted the ansibleroster branch July 18, 2018 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants