Update list_hosts.py #71
Conversation
Worked with engineering to add support for adding the container count with the host names and also for showing a larger number of hosts. The original script only showed about 10 hosts per environment.
examples/list_hosts.py
Outdated
| hostName = metrics[0] | ||
| count = metrics[1] | ||
| output.append('%s\t%d' % (hostName, count)) | ||
| print '\n'.join(output) |
There was a problem hiding this comment.
We can maybe add here a one liner, would this work?:
print('\n'.join(["%s\t%s" % (data['d'][0], data['d'][1]) for data in res[1]['data']])
There was a problem hiding this comment.
It would definitively work.
However, I would avoid patterns that could be a little cryptic, especially in the context of these examples that serve the purpose of documenting what can be done and how. For instance, hostName = metrics[0] would be unnecessary in most cases, here the goal was to explain that metrics is an array, and the first element is the hostName value related to the first metric specified in the list.
Hope this makes sense :-)
examples/list_hosts.py
Outdated
| # | ||
| # Instantiate the SDC client | ||
| sdclient = SdcClient(sdc_token) | ||
| metrics = [{"id": "host.hostName"}, {"id": "container.count","aggregations":{"time":"avg","group":"avg"}}] |
There was a problem hiding this comment.
I would write this list in multiple lines, so each metric we are querying is clearer:
metrics = [
{"id": "host.hostName"},
{
"id": "container.count",
"aggregations":{"time":"avg","group":"avg"}
},
]There was a problem hiding this comment.
Very good idea.
One thing we should investigate is using a "prettier" configuration that (either manually or automatically) can enforce some guidelines like the one you suggested. As much as I like consistency, I don't like leaving the burden of enforcing consistency in the hands of every developer. Let's work on it outside the scope of this PR ;-)
|
@dalejrodriguez @figarocorso I made few changes. Could you please review the changes to see if they still make sense? Thanks! |
Worked with engineering to add support for adding the container count with the host names and also for showing a larger number of hosts. The original script only showed about 10 hosts per environment.