Skip to content

Conversation

@originalgremlin
Copy link

Signed-off-by: Barry Shapira [email protected]

@originalgremlin
Copy link
Author

originalgremlin commented Dec 12, 2018

Adding the arguments to swarm init was relatively straightforward. I do wonder how the maintainers will feel about adding these arguments at all: other parts of the docker-py swarm API rely on a docker engine API version of 1.24, whereas these features are from a much more recent docker engine API of 1.39.

If any maintainers have ideas on how best to handle modernization of the swarm part of this repo I would be happy to help round out the codebase and truly "let you do anything the docker command does".

@shin-
Copy link
Contributor

shin- commented Dec 13, 2018

Thanks for submitting a PR @originalgremlin !

FWIW, the library already has a fairly established way to handle parameters added in newer versions of the API. see here or here for example. Please make sure to adopt the same idiom for this PR so we can move forward!

Also corrected a documentation error: the default API version from
constants is currently 1.35, not 1.30 as was sometimes listed.

Signed-off-by: Barry Shapira <[email protected]>
@originalgremlin
Copy link
Author

@shin- That makes sense. Thanks for pointing out the examples for how such things are handled.

Code updated and pushed. Has been tested a fair amount in my local environment. Did my best to stick with the project style though you may well have suggestions for things that I missed.

@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "2200-swarm-default-addr-pool" [email protected]:originalgremlin/docker-py.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354056040
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

Signed-off-by: Barry Shapira <[email protected]>
@originalgremlin originalgremlin force-pushed the 2200-swarm-default-addr-pool branch from d74d652 to 6e21b83 Compare December 14, 2018 16:49
The integration tests require restarting the swarm once for each
test.  I had done so manually with self.init_swarm(force_new_cluster=True)
but that wasn't resetting the swarm state correctly.  The usual
test teardown procedure cleans up correctly.

Signed-off-by: Barry Shapira <[email protected]>
@originalgremlin
Copy link
Author

@shin- Should be ready to go now. All tests pass and all protocol followed to the best of my ability. Let me know if there's any other polishing I should do.

@originalgremlin
Copy link
Author

@shin- Squeaky wheel ping. Anything missing or needing improvement in order to get this PR merged?

Copy link
Member

@chris-crone chris-crone left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @originalgremlin! Just a couple of minor comments.


@utils.minimum_version('1.24')
def init_swarm(self, advertise_addr=None, listen_addr='0.0.0.0:2377',
default_addr_pool=None, subnet_size=None,
Copy link
Member

Choose a reason for hiding this comment

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

Should add new parameters at the end of the list so that callers of init_swarm that use ordered (i.e.: unnamed) parameters continue to work as expected.

e.g.: An existing function could call:

s.init_swarm('192.168.0.1', '0.0.0.0:2377', True)

Which would now break.

get_unlock_key.__doc__ = APIClient.get_unlock_key.__doc__

def init(self, advertise_addr=None, listen_addr='0.0.0.0:2377',
default_addr_pool=None, subnet_size=None,
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here for parameters.

is used. Default: '0.0.0.0:2377'
default_addr_pool (list of strings): Default Address Pool specifies
default subnet pools for global scope networks. Each pool
should be specified as a CIDR block, like '10.0.0.0/16'.
Copy link
Member

Choose a reason for hiding this comment

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

nit: Might be good to use the default as the example.

hannseman pushed a commit to hannseman/docker-py that referenced this pull request Mar 22, 2019
hannseman pushed a commit to hannseman/docker-py that referenced this pull request Mar 22, 2019
hannseman added a commit to hannseman/docker-py that referenced this pull request Mar 22, 2019
hannseman pushed a commit to hannseman/docker-py that referenced this pull request Mar 22, 2019
hannseman added a commit to hannseman/docker-py that referenced this pull request Mar 22, 2019
hannseman added a commit to hannseman/docker-py that referenced this pull request Mar 22, 2019
hannseman added a commit to hannseman/docker-py that referenced this pull request Mar 22, 2019
Following docker#2201 (review)

Co-authored-by: hannseman <[email protected]>
Co-authored-by: bluikko

Signed-off-by: Hannes Ljungberg <[email protected]>
hannseman added a commit to hannseman/docker-py that referenced this pull request Mar 22, 2019
Co-authored-by: hannseman <[email protected]>
Co-authored-by: bluikko <[email protected]>

Following docker#2201 (review)

Signed-off-by: Hannes Ljungberg <[email protected]>
hannseman added a commit to hannseman/docker-py that referenced this pull request Mar 22, 2019
Co-authored-by: hannseman <[email protected]>
Co-authored-by: bluikko <[email protected]>

Following docker#2201 (review)

Signed-off-by: Hannes Ljungberg <[email protected]>
hannseman added a commit to hannseman/docker-py that referenced this pull request Mar 22, 2019
Co-authored-by: hannseman <[email protected]>
Co-authored-by: bluikko <[email protected]>

Following docker#2201 (review)

Signed-off-by: Hannes Ljungberg <[email protected]>
hannseman added a commit to hannseman/docker-py that referenced this pull request Mar 22, 2019
Co-authored-by: hannseman <[email protected]>
Co-authored-by: bluikko <[email protected]>

Following docker#2201 (review)

Signed-off-by: Hannes Ljungberg <[email protected]>
hannseman added a commit to hannseman/docker-py that referenced this pull request Mar 22, 2019
Co-authored-by: Hannes Ljungberg <[email protected]>
Co-authored-by: bluikko <[email protected]>

Following docker#2201 (review)

Signed-off-by: Hannes Ljungberg <[email protected]>
hannseman added a commit to hannseman/docker-py that referenced this pull request Mar 22, 2019
Following docker#2201 (review)

Signed-off-by: Hannes Ljungberg <[email protected]>
Co-authored-by: Hannes Ljungberg <[email protected]>
Co-authored-by: bluikko <[email protected]>
@ijc ijc closed this in #2287 Mar 25, 2019
ulyssessouza pushed a commit to ulyssessouza/docker-py that referenced this pull request Mar 26, 2019
Following docker#2201 (review)

Signed-off-by: Hannes Ljungberg <[email protected]>
Co-authored-by: Hannes Ljungberg <[email protected]>
Co-authored-by: bluikko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants