Code review comment for lp:~axwalk/juju-core/lp1237709-bootstrap-placement

Revision history for this message
Ian Booth (wallyworld) wrote :

LGTM with a suggestion.
Also a drive by fix for
// Directive is a scope-specific placement idrective

would be great.

https://codereview.appspot.com/91740043/diff/1/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/91740043/diff/1/cmd/juju/bootstrap.go#newcode107
cmd/juju/bootstrap.go:107: if err != instance.ErrPlacementScopeMissing {
What happens if there's a different error other than
ErrPlacementScopeMissing?
In that case we should just return that other error shouldn't we? ie
there's a difference between an unsupported placement directive and a
placement directive that doesn't parse due to some other error. Even if
there's no other error that can be returned (yet), reworking the
conditional would make it explicit that we are only interested in a
specific case.

https://codereview.appspot.com/91740043/

« Back to merge proposal