Code review comment for lp://staging/~fwereade/juju-core/environs-rework-instance-launch

Revision history for this message
Roger Peppe (rogpeppe) wrote :

a few comments, incomplete - publishing just in the aid of continuing
online conversation.

https://codereview.appspot.com/7394048/diff/2001/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/7394048/diff/2001/environs/bootstrap.go#newcode44
environs/bootstrap.go:44: type WriteCACertFunc func(name string, cert,
key []byte) error
i'm not entirely sure that this justifies its own type.

https://codereview.appspot.com/7394048/diff/2001/environs/bootstrap.go#newcode54
environs/bootstrap.go:54: if bootstrapped, err := e.IsBootstrapped();
err != nil {
it's a pity that this makes the bootstrap process fundamentally racy
(previously it was possible for a given provider to implement Bootstrap
in a race-free way).

https://codereview.appspot.com/7394048/diff/2001/environs/bootstrap.go#newcode71
environs/bootstrap.go:71: tools, err := PutTools(e.Storage(), nil)
this is slightly questionable, i think. previously, the storage methods
in Environ were not required to work before Bootstrap was called, so the
Bootstrap method was free to set up any storage required. we're changing
that here.

i wonder if we should have a separate Environ.Init method which makes
sure that the environment is set up to be used. It could be that method
that lets us know if the environment is already bootstrapped.

Environ.PrepareForBootstrap, perhaps? then that would leave a provider
free to hold some kind of lock between PrepareForBootstrap and... when
the environment is bootstrapped, which i guess it doesn't know.

https://codereview.appspot.com/7394048/

« Back to merge proposal