task_stats bugfix and query improvements. #105
Conversation
|
I think this looks good. @Rob-Johnson can you give a quick second opinion? @oilbeater can you address the lint failures? |
|
Fix pep8 issues, is there any way to run test locally, so I can test myself before push code. |
|
In theory you can run "make itests"? But it is a little tricky because you need docker and junk. |
|
Or just copy+paste what travis is doing: https://github.com/thefactory/marathon-python/blob/master/.travis.yml#L15 |
|
Any feedback on this? |
|
hey @oilbeater: It's a public holiday today, but I'll get to this first thing tomorrow morning UTC. |
|
|
||
| :returns: application | ||
| :rtype: :class:`marathon.models.app.MarathonApp` | ||
| """ | ||
| params = {'embed': 'apps.tasks'} if embed_tasks else {} | ||
| params = {} | ||
| embed_param = self._parse_embed_params() |
There was a problem hiding this comment.
I know it's significantly more verbose, but I'd much prefer you do the following to using inspect:
embed_params = {
'app.tasks': embed_tasks,
'app.counts': embed_counts,
'app.deployments': embed_deployments
....
}
filtered_embed_params = { k: v for k,v in embed_params if v }
params['embed'] = filtered_embed_params
Personally I think it makes it more explicit what is 'really' happening here, rather than hiding that complexity in the _parse_embed_params function (and it introspecting the current frame).
There was a problem hiding this comment.
I'm going to second @Rob-Johnson's request here.
The frame-inspection is pretty magical and it can be avoided, eh I guess we would have to copy-paste one time? (once for get_app and once for list_apps)
There was a problem hiding this comment.
Agreed that inspect is magical to achieve less redundant code, otherwise these params code should appear twice in get_app and list_apps. If these two methods are not merged into one, it seems hard to avoid redundancy. If the redundancy is acceptable, I am ok to change the code in this way.
There was a problem hiding this comment.
I think it's fine @oilbeater
|
I've left a comment, but I'll leave it to @solarkennedy to decide if he wants to block shipping on this. lg2m otherwise, ty for your contributions to this @oilbeater, it's appreciated! |
Fix task_stats bugs in MarathonApp init that wrongly use version_info. Add more query parameters to list_apps and get_app to get more info. Add readiness_check_results to MarathonApp when query embed with app.readiness marathon will return readinessCheckResults field.
|
shipit |
|
Ty @oilbeater! |
Fix task_stats bugs in MarathonApp init that wrongly use version_info.
Add more query parameters to list_apps and get_app to get more info.
Add readiness_check_results to MarathonApp when query embed with app.readiness marathon will return readinessCheckResults field.