Code review comment for lp://staging/~axwalk/juju-core/wire-up-prechecker

Revision history for this message
William Reade (fwereade) wrote :

Sorry this one has hung around so long, but I think it's basically
there. Last round?

https://codereview.appspot.com/14032043/diff/19001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/14032043/diff/19001/juju/conn.go#newcode171
juju/conn.go:171: func (c *Conn) setPrechecker() error {
Good point in your comment that I think was around here. Let's use this
to set the environ on the Conn itself -- I can't think of a good reason
not to tbh.

https://codereview.appspot.com/14032043/diff/19001/provider/azure/environ.go
File provider/azure/environ.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/azure/environ.go#newcode83
provider/azure/environ.go:83: var _ state.Prechecker =
(*azureEnviron)(nil)
Don't think we need this now it's in the Environ interface

https://codereview.appspot.com/14032043/diff/19001/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/ec2/ec2.go#newcode69
provider/ec2/ec2.go:69: var _ state.Prechecker = (*environ)(nil)
ditto

https://codereview.appspot.com/14032043/diff/19001/provider/ec2/local_test.go
File provider/ec2/local_test.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/ec2/local_test.go#newcode196
provider/ec2/local_test.go:196: prechecker, ok := env.(state.Prechecker)
unnecessary again?

https://codereview.appspot.com/14032043/diff/19001/provider/null/environ.go
File provider/null/environ.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/null/environ.go#newcode165
provider/null/environ.go:165: func (*nullEnviron)
PrecheckContainer(series string, kind instance.ContainerType) error {
Hmm. Unless we can give containers their own addresses, we might need to
error out here too.

https://codereview.appspot.com/14032043/diff/19001/provider/openstack/local_test.go
File provider/openstack/local_test.go (right):

https://codereview.appspot.com/14032043/diff/19001/provider/openstack/local_test.go#newcode251
provider/openstack/local_test.go:251: prechecker, ok :=
env.(state.Prechecker)
again not necessary I think

https://codereview.appspot.com/14032043/diff/19001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14032043/diff/19001/state/state.go#newcode236
state/state.go:236: // in which case we do not want to precheck it.
Is this really the best way to get it? Seems a little bit roundabout.
I'd prefer to use InstanceId, really, it seems a bit clearer to me.

https://codereview.appspot.com/14032043/

« Back to merge proposal