|
|
|
provider/openstack: Add network config option
Resolve issue with running juju on openstack setups with
multiple networks, by allowing the user to supply a
network label or id that juju should use for its machines.
https://code.launchpad.net/~gz/juju-core/openstack_network_config_1241674/+merge/203114
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 6
Patch Set 2 : provider/openstack: Add network config option #
Total comments: 4
Patch Set 3 : provider/openstack: Add network config option #
Total messages: 5
|
gz
Please take a look.
|
11 years, 11 months ago (2014年01月24日 16:47:12 UTC) #1 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
quick comments -- thoughts? https://codereview.appspot.com/52710048/diff/1/provider/openstack/config.go File provider/openstack/config.go (right): https://codereview.appspot.com/52710048/diff/1/provider/openstack/config.go#n... provider/openstack/config.go:42: "network": "", this feels like maybe it should be an Omit? https://codereview.appspot.com/52710048/diff/1/provider/openstack/config.go#n... provider/openstack/config.go:95: return c.attrs["network"].(string) and this a `,ok`? https://codereview.appspot.com/52710048/diff/1/provider/openstack/local_test.go File provider/openstack/local_test.go (right): https://codereview.appspot.com/52710048/diff/1/provider/openstack/local_test.... provider/openstack/local_test.go:364: "network": "f81d4fae-7dec-11d0-a765-00a0c91e6bf6", a thought: default-network? private-network? https://codereview.appspot.com/52710048/diff/1/provider/openstack/local_test.... provider/openstack/local_test.go:371: c.Assert(err, gc.ErrorMatches, "(?s)cannot run instance: .*itemNotFound.*") I'd like to see a more precise error check here really.
Please take a look. https://codereview.appspot.com/52710048/diff/1/provider/openstack/local_test.go File provider/openstack/local_test.go (right): https://codereview.appspot.com/52710048/diff/1/provider/openstack/local_test.... provider/openstack/local_test.go:364: "network": "f81d4fae-7dec-11d0-a765-00a0c91e6bf6", On 2014年01月24日 16:53:49, fwereade wrote: > a thought: default-network? private-network? Yeah, config named 'network' is bad, but I think config for this at all is bad. And it should take multiple values. I am resolved to leave some bikeshedding for post 1.17.1 over this. :) https://codereview.appspot.com/52710048/diff/1/provider/openstack/local_test.... provider/openstack/local_test.go:371: c.Assert(err, gc.ErrorMatches, "(?s)cannot run instance: .*itemNotFound.*") On 2014年01月24日 16:53:49, fwereade wrote: > I'd like to see a more precise error check here really. Sadly our errors through goose are massively verbose, and the real errors from (older at least) openstack versions are actually this impresice. It might be an argument for doing more prechecking even when a valid uuid is supplied.
LGTM with a couple small points. https://codereview.appspot.com/52710048/diff/20001/provider/openstack/provide... File provider/openstack/provider.go (right): https://codereview.appspot.com/52710048/diff/20001/provider/openstack/provide... provider/openstack/provider.go:79: # networks specifies the network label or uuid to bring machines up on, in s/networks/network https://codereview.appspot.com/52710048/diff/20001/provider/openstack/provide... provider/openstack/provider.go:642: var uuidRegexp = regexp.MustCompile(uuidPattern) Seems like there ought to be a function that verifies guids in Go instead of hand-writing a regex...
Please take a look. https://codereview.appspot.com/52710048/diff/20001/provider/openstack/provide... File provider/openstack/provider.go (right): https://codereview.appspot.com/52710048/diff/20001/provider/openstack/provide... provider/openstack/provider.go:79: # networks specifies the network label or uuid to bring machines up on, in On 2014年01月24日 20:12:45, nate.finch wrote: > s/networks/network Will fix... I did plan to allow supplying multiple initially, but we'll leave that for later. https://codereview.appspot.com/52710048/diff/20001/provider/openstack/provide... provider/openstack/provider.go:642: var uuidRegexp = regexp.MustCompile(uuidPattern) On 2014年01月24日 20:12:45, nate.finch wrote: > Seems like there ought to be a function that verifies guids in Go instead of > hand-writing a regex... Yes, yes there should. The best I could find was: <https://github.com/nu7hatch/gouuid/blob/master/uuid.go>