-
Notifications
You must be signed in to change notification settings - Fork 70
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
added generic inventory filter minor fixes to other inventory files #62
added generic inventory filter minor fixes to other inventory files #62
Conversation
…n InventoryFilter package updated linchpin_configfile
@@ -19,7 +19,6 @@ def get_host_ips(self, topo): | |||
|
|||
def get_inventory(self, topo, layout): | |||
inventory = ConfigParser(allow_no_value=True) | |||
layout_hosts = self.get_layout_hosts(layout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed unused variable
…esses issue CentOS-PaaS-SIG#59 Regression introduced in schema_check.py
@@ -79,9 +79,9 @@ def check_file_paths(module, *args): | |||
|
|||
def validate_grp_names(data): | |||
res_grps = data['resource_groups'] | |||
res_grp_vars = data['resource_group_vars'] | |||
res_grp_vars = data['resource_group_vars'] if 'resource_group_vars' in data.keys() else [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd highly recommend list comprehensions here. It's cleaner.
http://stackoverflow.com/questions/4406389/if-else-in-a-list-comprehension
res_grp_names = [ x['resource_group_name'] for x in res_grps ] | ||
res_grp_vars = [ x['resource_group_name'] for x in res_grp_vars ] | ||
res_grp_vars = [ x['resource_group_name'] for x in res_grp_vars ] if len(res_grp_vars) > 0 else [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd highly recommend list comprehensions here. It's cleaner.
http://stackoverflow.com/questions/4406389/if-else-in-a-list-comprehension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@herlo In the above scenario , i am using if else statement as a ternery operator ,
http://book.pythontips.com/en/latest/ternary_operators.html
res_grp_vars are defaulted to empty list , if not mentioned.
For readability it can be changed to as follows :
if len(res_grp_vars) > 0:
# essentially all dicts of res_grp_vars into a list
res_grp_vars = [ x['resource_group_name'] for x in res_grp_vars ]
else:
res_grp_vars = []
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
res_grp_vars = [ x['resource_group_name'] if len(res_grp_vars) > 0 else [] for x in res_grp_vars]
The above seems like it would handle this, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no it creates a nested list when res_grp_vars are empty
when len(res_grp_vars) == 0 it results in following list
[ [] ]
class GenericInventory(InventoryFilter): | ||
def __init__(self): | ||
InventoryFilter.__init__(self) | ||
self.filter_classes = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I continue to question this list of classes. Could it be pulled from the linchpin_config.yml or elsewhere? I'd like to see this be more dynamic. Or possibly, it could be pulled from the classes that have Inventory.py in the python path?
I have code that can do the instantiation of the classes from a config file or the like in a pythonic way, so that shouldn't be too troublesome to implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed ,
Whats you call on design which is similar to as follows :
https://github.com/apache/libcloud/blob/trunk/libcloud/compute/providers.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with this concept.
…n InventoryFilter package updated linchpin_configfile
…esses issue CentOS-PaaS-SIG#59 Regression introduced in schema_check.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Only some minor change recommendations. Good work SK!
return inv['host_groups'].keys() | ||
count = 0 | ||
for host_group in inv['hosts']: | ||
count += inv['hosts'][host_group]['count'] if 'count' in inv['hosts'][host_group] else 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could use a list comprehension here.
for host_name in layout['hosts']: | ||
count = layout['hosts'][host_name]['count'] | ||
#if type(layout['hosts'][host_name]) is dict: | ||
count = layout['hosts'][host_name]['count'] if 'count' in layout['hosts'][host_name] else 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
list comprehension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good sk! Nice work!
No description provided.