state: Machine.InstanceId returns bool

state: Machine.InstanceId returns bool

It brings it into line with other similar accessors.

https://code.launchpad.net/~rogpeppe/juju-core/221-instanceid-bool/+merge/149097

(do not edit description out of merge proposal)

[rog, 2013-02-18 16:38:26]

Please take a look.

[dave_cheney.net, 2013-02-18 22:29:55]

LGTM. Thank you.

https://codereview.appspot.com/7327050/diff/2001/cmd/juju/ssh.go
File cmd/juju/ssh.go (right):

https://codereview.appspot.com/7327050/diff/2001/cmd/juju/ssh.go#newcode98
cmd/juju/ssh.go:98: instid, ok := machine.InstanceId()
if instid, ok := ... ; ok { 
   ...
}

Sorry, this was my mistake initially

https://codereview.appspot.com/7327050/diff/2001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/7327050/diff/2001/cmd/juju/status.go#newcode120
cmd/juju/status.go:120: instid, ok := m.InstanceId()
same

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

https://codereview.appspot.com/7327050/diff/2001/cmd/jujud/machine_test.go#ne...
cmd/jujud/machine_test.go:172: _, ok := m1.InstanceId()
if _, ok := ... ; ok {
   continue
}

https://codereview.appspot.com/7327050/diff/2001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/7327050/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:121: // InstanceId returns the provider specific instance
id for this machine.
Please update the comment to match state.Machine.InstanceId()

https://codereview.appspot.com/7327050/diff/2001/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/7327050/diff/2001/worker/firewaller/firewaller...
worker/firewaller/firewaller.go:268: if !ok {
Should this use state.NotFoundf ?

https://codereview.appspot.com/7327050/diff/2001/worker/firewaller/firewaller...
worker/firewaller/firewaller.go:441: if !ok {
same

https://codereview.appspot.com/7327050/diff/2001/worker/provisioner/provision...
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/7327050/diff/2001/worker/provisioner/provision...
worker/provisioner/provisioner_test.go:161: _, ok := m.InstanceId()
if _, ok := ... ; !ok {
  ...
}

[jameinel, 2013-02-19 10:50:02]

I think the specific of returning a bool to indicate "string is not empty" is a
bit odd, and you lose the ability to define an error a the time where you
actually read it.

But most callers do treat the error as a bool.

So LGTM, though I'm not 100% sold on the idea.

[dimitern, 2013-02-19 10:58:08]

LGTM, and I agree with dfc's suggestions.

[dave_cheney.net, 2013-02-19 11:16:09]

I think the argument here is the document is already in memory, so any error
would have already been encountered. Back in the zk days every field accessor
could return an error, which sucked. Now that we've fixed all this, machineId
sticks out as there is one condition that can occur naturally without being an
error. 

On 19/02/2013, at 21:50, john.meinel@canonical.com wrote:

> I think the specific of returning a bool to indicate "string is not
> empty" is a bit odd, and you lose the ability to define an error a the
> time where you actually read it.
> 
> But most callers do treat the error as a bool.
> 
> So LGTM, though I'm not 100% sold on the idea.
> 
> https://codereview.appspot.com/7327050/

[rog, 2013-02-19 11:30:47]

*** Submitted:

state: Machine.InstanceId returns bool

It brings it into line with other similar accessors.

R=dfc, jameinel, dimitern
CC=
https://codereview.appspot.com/7327050

https://codereview.appspot.com/7327050/diff/2001/cmd/juju/ssh.go
File cmd/juju/ssh.go (right):

https://codereview.appspot.com/7327050/diff/2001/cmd/juju/ssh.go#newcode98
cmd/juju/ssh.go:98: instid, ok := machine.InstanceId()
On 2013/02/18 22:29:55, dfc wrote:
> if instid, ok := ... ; ok { 
>    ...
> }
> 
> Sorry, this was my mistake initially

done, but i consider it a matter of taste rather than a mistake. i'm happy with
both forms.

i've left you with a note about the real bug in this loop which, to be fair, you
may well not have been responsible for :-)

https://codereview.appspot.com/7327050/diff/2001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/7327050/diff/2001/cmd/juju/status.go#newcode120
cmd/juju/status.go:120: instid, ok := m.InstanceId()
On 2013/02/18 22:29:55, dfc wrote:
> same

i'll leave this as is actually - the slightly less dense code is more readable
IMHO.

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

https://codereview.appspot.com/7327050/diff/2001/cmd/jujud/machine_test.go#ne...
cmd/jujud/machine_test.go:172: _, ok := m1.InstanceId()
On 2013/02/18 22:29:55, dfc wrote:
> if _, ok := ... ; ok {
>    continue
> }

done, slightly differently.

https://codereview.appspot.com/7327050/diff/2001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/7327050/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:121: // InstanceId returns the provider specific instance
id for this machine.
On 2013/02/18 22:29:55, dfc wrote:
> Please update the comment to match state.Machine.InstanceId()

Done.

https://codereview.appspot.com/7327050/diff/2001/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/7327050/diff/2001/worker/firewaller/firewaller...
worker/firewaller/firewaller.go:268: if !ok {
On 2013/02/18 22:29:55, dfc wrote:
> Should this use state.NotFoundf ? 

it can't yet, as that CL is dependent on this one. will change subsequently.
added a TODO.

https://codereview.appspot.com/7327050/diff/2001/worker/firewaller/firewaller...
worker/firewaller/firewaller.go:441: if !ok {
On 2013/02/18 22:29:55, dfc wrote:
> same

same

https://codereview.appspot.com/7327050/diff/2001/worker/provisioner/provision...
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/7327050/diff/2001/worker/provisioner/provision...
worker/provisioner/provisioner_test.go:161: _, ok := m.InstanceId()
On 2013/02/18 22:29:55, dfc wrote:
> if _, ok := ... ; !ok {
>   ...
> }

done, slightly differently.