Code review comment for lp://staging/~wallyworld/juju-core/add-machine-parent-getter

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

LGTM with ParentId as suggested.

https://codereview.appspot.com/10203043/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/10203043/diff/1/state/machine.go#newcode241
state/machine.go:241: return nil, nil
There's a general expectation that a func returning (*Foo, error) will
give you an error iff it can't give you a non-nil *Foo, so I don't
really like this.

https://codereview.appspot.com/10203043/diff/1/state/machine.go#newcode243
state/machine.go:243: return m.st.Machine(parentId)
On 2013/06/11 22:47:06, thumper wrote:
> Unfortunately this is making an assumption that doesn't always hold.

> If I am the provisioner, and I have a state.Machine instance, I won't
> necessarily have access to state.

> Can we have something that returns ParentId() ?

> While this method is OK now, I'm wondering how the API changes are
going to
> impact it.

Ideally, I think, `ParentId() (string, bool)`, in the name of convention
wrt the state package.

https://codereview.appspot.com/10203043/

« Back to merge proposal