Skip to content
This repository has been archived by the owner on Nov 19, 2020. It is now read-only.

Allow specifying multiple Marathon addresses and failover to each sequentially #74

Merged
merged 7 commits into from
Nov 24, 2016

Conversation

JayH5
Copy link
Contributor

@JayH5 JayH5 commented Nov 22, 2016

Fixes #48.

I basically copied what the "official" Marathon Python client does but in async.

@codecov-io
Copy link

codecov-io commented Nov 22, 2016

Current coverage is 99.75% (diff: 100%)

Merging #74 into develop will increase coverage by 0.01%

@@            develop        #74   diff @@
==========================================
  Files            17         18     +1   
  Lines          1570       1664    +94   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1566       1660    +94   
  Misses            4          4          
  Partials          0          0          

Powered by Codecov. Last update 20c5a3c...6e606ca

Copy link
Member

@jerith jerith left a comment

Choose a reason for hiding this comment

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

Looks pretty good so far, just a couple of minor questions.

@@ -532,7 +605,7 @@ def test_get_json_field(self):

request = yield self.requests.get()
self.assertThat(request, HasRequestProperties(
method='GET', url=self.uri('/my-path')))
method='GET', url='http://localhost:8080/my-path'))
Copy link
Member

Choose a reason for hiding this comment

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

Can we make self.uri() work for the common case instead of having to explicitly provide the full URI everywhere?

usage: marathon-acme [-h] [-a ACME] [-m MARATHON] [-l LB [LB ...]] [-g GROUP]
[--listen LISTEN]
usage: marathon-acme [-h] [-a ACME] [-m MARATHON [MARATHON ...]]
[-l LB [LB ...]] [-g GROUP] [--listen LISTEN]
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a huge fan of space-separated opts like -m foo bar baz, because it looks like the extras are positional args.

Can we make these comma-separated or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I maybe do this in a separate PR and do it for --lb as well?

Copy link
Member

Choose a reason for hiding this comment

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

Definitely.

Copy link
Member

@jerith jerith left a comment

Choose a reason for hiding this comment

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

👍

@JayH5 JayH5 merged commit eaf0040 into develop Nov 24, 2016
@JayH5 JayH5 deleted the marathon-failover branch January 4, 2017 11:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants