Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let the api to choose the default network driver. #17504

Merged
merged 1 commit into from Oct 30, 2015

Conversation

calavera
Copy link
Contributor

That way swarm can understand the user's intention.

@tiborvass it turns out that we already have a bunch of tests in integration-cli/docker_api_network_test.go that already assume the driver is an empty string and let the daemon to select bridge as default network. I added an extra test here, but I don't think it's necessary.

Closes #17500

Signed-off-by: David Calavera david.calavera@gmail.com

@tiborvass
Copy link
Contributor

@calavera that's interesting, then why wasn't it caught?

@tiborvass
Copy link
Contributor

@calavera needs rebase :D #thingsgotoofast

@calavera calavera added this to the 1.9.0 milestone Oct 29, 2015
@calavera calavera force-pushed the default_network_driver branch 3 times, most recently from 953e0aa to acf5acf Compare October 29, 2015 21:04
@calavera
Copy link
Contributor Author

Rebased, I removed the test because we have others that do the same. See:

https://github.com/docker/docker/blob/master/integration-cli/docker_api_network_test.go#L26

@tiborvass because we have a check for empty in https://github.com/docker/docker/blob/master/daemon/network.go#L91

@mavenugo
Copy link
Contributor

thanks @calavera. LGTM.

I honestly don't remember why I made bridge as default at the client side.
But as you rightly spotted, it is not required.

@tiborvass
Copy link
Contributor

@mavenugo it's not that it's not required, it's a bug if it's there :)

@mavenugo
Copy link
Contributor

@tiborvass okay. i remember why I added it :) and it is not a bug. It was addressing this comment : #16645 (comment)

@calavera
Copy link
Contributor Author

having the right default in the help output makes sense. I don't know how we can check who set that value 😔

@tiborvass
Copy link
Contributor

@calavera if we don't already, we could have an INFO log in the daemon logs to tell which driver was selected, better than nothing.

@calavera
Copy link
Contributor Author

Okay, I found how to solve this issue for real, yay for reading the docs!

It turns out you can detect whether a user set a flag or not with flagSet.IsSet. So, we send an empty string if the user didn't set bridge as the driver. That way swarm knows what to do with the value.

@tiborvass, @mavenugo, @vieux please, take a look.

@calavera
Copy link
Contributor Author

These are the parameters we send to the api when I run docker network create --driver bridge foo:

Calling POST /v1.22/networks/create
form data: map["options":map[] "name":"newbridge" "check_duplicate":%!q(bool=true) "driver":"bridge" "ipam":map["driver":"default" "config":[]]]

And this is what we send when I run docker network create bar:

Calling POST /v1.22/networks/create
form data: map["options":map[] "name":"bridge2" "check_duplicate":%!q(bool=true) "driver":"" "ipam":map["driver":"default" "config":[]]]

See that when I(the user) set the driver to bridge, we send that value to the api, but we don't send anything if I don't set the driver.

Moreover, docker network create -h still shows bridge as the default option:

Usage:  docker network create [OPTIONS] NETWORK-NAME

Creates a new network with a name specified by the user

  --aux-address=map[]      auxiliary ipv4 or ipv6 addresses used by Network driver
  -d, --driver=bridge      Driver to manage the Network
  --gateway=[]             ipv4 or ipv6 Gateway for the master subnet

@vieux
Copy link
Contributor

vieux commented Oct 29, 2015

even better thanks @lgtm

@tiborvass
Copy link
Contributor

@calavera nice, that's what we also do for --tls. If others are fine with this i'm fine too

That way swarm can understand the user's intention.

Signed-off-by: David Calavera <david.calavera@gmail.com>
@tiborvass
Copy link
Contributor

LGTM

@tiborvass
Copy link
Contributor

dunno what windows ci is doing, merging

tiborvass added a commit that referenced this pull request Oct 30, 2015
Let the api to choose the default network driver.
@tiborvass tiborvass merged commit d7bdbdf into moby:master Oct 30, 2015
@calavera calavera deleted the default_network_driver branch October 30, 2015 20:48
@tiborvass tiborvass removed their assignment Oct 31, 2015
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.

None yet

5 participants