Updated to support Marathon 0.9.1 with get_info() calls #59
Conversation
|
This PR seems a bit incomplete, did you add all the files? |
|
Nope, that's it. Is there something I should have done? |
|
I expected references to get_info? Are the changes to support it just zookeeper timeout params? Can you add support for 0.9.1 in the travis.yaml if it works? |
|
There were 5 keys added to the API between 0.9 and 0.9.1.
These were all the changes required to get it working (API additions cause kwargs exceptions thrown). (Now updated with the .travis.yml change) |
|
This is good, but can you actually add testing coverage for this function that we are expanding? (like calling the That way, when the api changes the next time, the integration tests will let us know right away as we attempt to support new versions. |
|
Oh yeah! Testing. That's a great idea. Let me add that. |
|
So, I couldn't get the tests to run locally (pkg_resources.DistributionNotFound: requests>=2.6.1,<2.7 when running |
| @@ -19,6 +19,11 @@ def working_marathon(context): | |||
| context.client = marathon.MarathonClient(marathon_connection_string) | |||
|
|
|||
|
|
|||
| @then(u'we get the marathon instance\'s info') | |||
There was a problem hiding this comment.
I don't see this ever get called?
See itests/marathon_python.feature and start a new scenario for querying marathon in general
|
Yea it looks like tox installs requests 2.7 and docker compose needs <2.7. I can't figure out how to make tox keep a version that docker compose likes. I guess we could pin it <2.7 in setup.py, but I shouldn't have to do that. Till then you can do to force a downgrade to something docker-compose can use. |
|
@solarkennedy That's what I get for not testing. Your workaround was perfect, test appears to be running and passing now! |
| @@ -2,6 +2,7 @@ Feature: marathon-python can create and list marathon apps | |||
|
|
|||
| Scenario: Trivial apps can be deployed | |||
| Given a working marathon instance | |||
| Then we get the marathon instance's info | |||
There was a problem hiding this comment.
I think it would be more readable if this was a separate scenario (especially since this doesn't really have much to do with "Trivial apps can be deployed").
There was a problem hiding this comment.
Yes please. Plus once we have a place for that sort of thing we can add other general information gathering methods, (get_leader, get_metrics, ping, etc)
|
Whoops, sorry about that. I've moved it into its own scenario now. |
| Scenario: Trivial apps can be deployed | ||
| Given a working marathon instance | ||
| Then we get the marathon instance's info |
There was a problem hiding this comment.
Can you remove this now? It isn't really related to trivial apps and there is no need for us to exercise this twice
|
That is stupidly embarrassing! Now removed. |
| :param str zk_state: | ||
| :param int zk_timeout: | ||
| """ | ||
|
|
||
| def __init__(self, zk=None, zk_future_timeout=None, zk_hosts=None, zk_path=None, zk_state=None, zk_timeout=None): | ||
| def __init__(self, zk=None, zk_future_timeout=None, zk_hosts=None, zk_max_versions=None,zk_path=None, |
There was a problem hiding this comment.
Sorry if I sound a bit anal, but do you mind adding a space between "zk_max_versions=None,zk_path=None"?
|
And, updated again. I'm starting to wonder if I can actually write code =) |
|
Thanks! |
Updated to support Marathon 0.9.1 with get_info() calls
No description provided.