Skip to content

Conversation

rmb938
Copy link
Contributor

@rmb938 rmb938 commented Oct 21, 2015

This addresses issue #815

These options are currently not documented but are still useful when creating networks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this doesn't need to exist inside the Client (doesn't use self), I think it should be moved to a function inside the docker.utils module instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, moving it now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it would be nice to have it renamed to create_ipam_config to be consistent with the currently existing create_host_config and create_container_config. 😄

@shin-
Copy link
Contributor

shin- commented Oct 21, 2015

Thanks for contributing!

I'm going to wait for the 1.9.0 release to merge this to make sure things don't change last minute.

In the meantime, having some tests and mentioning the added methods in the documentation would be a valuable improvement.

@rmb938
Copy link
Contributor Author

rmb938 commented Oct 21, 2015

Sounds good. Will start working on those.

@shin-
Copy link
Contributor

shin- commented Nov 11, 2015

Hi @rmb938 , do you mind rebasing this? We can take a closer look now that 1.9 is out.

@shin- shin- added this to the 1.7.0 milestone Nov 24, 2015
@rmb938
Copy link
Contributor Author

rmb938 commented Dec 5, 2015

@shin- Yup no problem. Sorry for the late reply I will get it done this weekend.

@rmb938
Copy link
Contributor Author

rmb938 commented Dec 26, 2015

@shin- sorry for the delay. I got everything rebased. if it all looks good I can squash all the commits for you.

Copy link
Contributor

Choose a reason for hiding this comment

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

pool_configs or [] would allow us to get rid of the first two lines here. Small readability improvement.

@shin-
Copy link
Contributor

shin- commented Jan 4, 2016

Thanks for updating. A few things to change in the docs and a small nitpick in the create_ipam_config function, then we should be good.

Feel free to squash it as well :)

Signed-off-by: Ryan Belgrave <rmb1993@gmail.com>
@rmb938
Copy link
Contributor Author

rmb938 commented Jan 5, 2016

@shin- how does that look

@shin-
Copy link
Contributor

shin- commented Jan 5, 2016

Thanks, looking great - LGTM!

@aanand
Copy link
Contributor

aanand commented Jan 5, 2016

LGTM

aanand added a commit that referenced this pull request Jan 5, 2016
allow custom ipam options when creating networks
@aanand aanand merged commit 62d9964 into docker:master Jan 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants