Code review comment for lp:~jameinel/juju-core/api-set-creds-1199915

Looking very nice, thanks for this; I have a couple of questions.

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/machine.go#newcode145
cmd/jujud/machine.go:145: // TODO: Once we can reliably trust that we
have API passwords
Give this a TODO(jam), it's nice to be able to know who to ask even when
blame info is lost through unrelated changes.

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/machine_test.go#newcode73
cmd/jujud/machine_test.go:73: err = conf.Write()
thanks, good catch

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation.go
File cmd/jujud/upgradevalidation.go (right):

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation.go#newcode110
cmd/jujud/upgradevalidation.go:110: }
Can you use State.APIAddresses for this? Or something like that...
defined in state/open.go I think.

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation.go#newcode134
cmd/jujud/upgradevalidation.go:134: // This is unexpected as all
AgentState objects (Machine and Unit)
On 2013/07/15 13:31:43, mue wrote:
> Isn't this worth a panic()? How does the system continue with this
error?

Yeah, I think this is a panic. Someone completely screwed up, do not
pass go, do not collect $200.

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation.go#newcode165
cmd/jujud/upgradevalidation.go:165: if err :=
setter.SetPassword(password); err != nil {
Hmm. The original idea IIRC was that we save the oldPassword (for
recovery purposes), write the new password to disk, and then really set
the new password. Not quite sure this mechanism and that one are
properly joined-up?

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation_test.go
File cmd/jujud/upgradevalidation_test.go (right):

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation_test.go#newcode47
cmd/jujud/upgradevalidation_test.go:47: func (s
*UpgradeValidationMachineSuite) Create1_10Machine(c *gc.C)
(*state.Machine, *agent.Conf) {
On 2013/07/15 13:31:43, mue wrote:
> Needed a few seconds to interpret 1_10 right. ;) How about
Create1Dot10Machine?

-1 myself, but happy to follow consensus

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation_test.go#newcode226
cmd/jujud/upgradevalidation_test.go:226: func (s
*UpgradeValidationUnitSuite) TestEnsureAPIInfo111(c *gc.C) {
1_11 for consistency with the above?

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation_test.go#newcode247
cmd/jujud/upgradevalidation_test.go:247: func (s
*UpgradeValidationUnitSuite) TestEnsureAPIInfo111Noop(c *gc.C) {
ditto

https://codereview.appspot.com/11137044/