Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor lago ansible_hosts #544

Merged
merged 1 commit into from
May 23, 2017
Merged

Refactor lago ansible_hosts #544

merged 1 commit into from
May 23, 2017

Conversation

gbenhaim
Copy link
Member

  1. Every key in the init file can be used as a group.
    In order to create a unique name for each group, the
    value is prefixed with the series of keys and indexes that
    points to it (for example [vm-type=ovirt-host]).

  2. Added the option to specify 'groups' key in the domain definition,
    this key should point to a list of groups that the domain belongs to.

  3. Moved the logic to a new file called ansible.py (this way 'lago sdk'
    would be able to use it without the need for cmd.py).

For more info check out the description in cmd.py

closes #429

Signed-off-by: gbenhaim galbh2@gmail.com

@gbenhaim gbenhaim added this to the 0.39 milestone May 21, 2017
@gbenhaim gbenhaim force-pushed the refactor_ansible_hosts branch from 01a227f to 7ceaed3 Compare May 21, 2017 18:00
@gbenhaim gbenhaim requested a review from eedri May 21, 2017 18:00
lago/ansible.py Outdated

def get_inventory(self, keys=None):
"""
Create an Ansible inventory based on python object.
Copy link
Contributor

Choose a reason for hiding this comment

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

'Based on python object' is a bit unclear on what sort of object it expects.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

lago/ansible.py Outdated
@@ -0,0 +1,131 @@
from collections import defaultdict
Copy link
Contributor

Choose a reason for hiding this comment

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

The filename ansible.py might be problematic if we ever try in the future to use ansible's own API(i.e. import ansible), also for users of Lago(i.e. they might do from lago import ansible)
Similarly to how running ipython on my env inside lago/lago directory fails because it conflicts with cmd.py :/

Copy link
Member Author

Choose a reason for hiding this comment

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

done

lago/ansible.py Outdated
"""
inventory = defaultdict(list)
keys = keys or ['vm-type', 'groups', 'vm-provider']
vms = self.virt.get_vms().values()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can call this directly from the prefix no?
i.e.

self.prefix.get_vms().values()


The 'keys' parameter can be used to override the default groups,
for example to create a group which based on a 'service_provider',
--keys 'service_provider' should be added to this command.
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks :)

lago/cmd.py Outdated
for every item in that list (this is useful when you want to associate
a machine with more than one group).

The output of this command ansis printed to standard output.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: ansis

help='Path to the keys that should be used as groups',
default=['vm-type', 'groups', 'vm-provider'],
metavar='KEY',
nargs='*',
Copy link
Contributor

Choose a reason for hiding this comment

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

Just making sure - this change is compatible with the old API?(i.e. what OST Ansible uses).

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not.
Currently OST Ansible uses [ovirt-host], [ovirt-engine] while the new names
are [vm-type=ovirt-host], [vm-type=ovirt-engine].
Since any key can be used as a group, I have to keep it this way (in order to create a unique group name).

return self._spec['vm-type']

@property
def groups(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

docstrings

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@gbenhaim gbenhaim force-pushed the refactor_ansible_hosts branch from 7ceaed3 to 4806d99 Compare May 22, 2017 17:38
Copy link
Member

@machacekondra machacekondra left a comment

Choose a reason for hiding this comment

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

Looks nice.

1. Every key in the init file can be used as a group.
   In order to create a unique name for each group, the
   value is prefixed with the series of keys and indexes that
   points to it (for example [vm-type=ovirt-host]).

2. Added the option to specify 'groups' key in the domain definition,
   this key should point to a list of groups that the domain belongs to.

3. Moved the logic to a new file called ansible.py (this way 'lago sdk'
   would be able to use it without the need for cmd.py).

Signed-off-by: gbenhaim <galbh2@gmail.com>
@gbenhaim gbenhaim force-pushed the refactor_ansible_hosts branch from 4806d99 to b3a0bcc Compare May 23, 2017 09:16
@gbenhaim
Copy link
Member Author

ci merge please

@ovirt-infra ovirt-infra merged commit ecd23d3 into master May 23, 2017
@ovirt-infra ovirt-infra deleted the refactor_ansible_hosts branch May 23, 2017 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define ansible role per VM
4 participants