Code review comment for lp:~dave-cheney/pyjuju/go-provisioning-strawman

Thanks Rog. I'll address those in the pre req by adding machine.InstanceId() and pushing the logic into that state.

d

On 29/05/2012, at 18:44, Roger Peppe <email address hidden> wrote:

> this is looking nice.
> a few minor remarks below.
>
>
>
> https://codereview.appspot.com/6250068/diff/2001/cmd/jujud/provisioning.go
> File cmd/jujud/provisioning.go (right):
>
> https://codereview.appspot.com/6250068/diff/2001/cmd/jujud/provisioning.go#newcode17
> cmd/jujud/provisioning.go:17: const PROVIDER_MACHINE_ID =
> "provider-machine-id"
> s/PROVIDER_MACHINE_ID/providerMachineId/
>
> UNDERSCORED_CAPS aren't conventional.
>
> https://codereview.appspot.com/6250068/diff/2001/cmd/jujud/provisioning.go#newcode120
> cmd/jujud/provisioning.go:120: return fmt.Errorf("machine-%010d already
> reports a provider id %q, skipping", m.Id(), id)
> s/machine-%010d/machine %d/
>
> https://codereview.appspot.com/6250068/diff/2001/cmd/jujud/provisioning.go#newcode208
> cmd/jujud/provisioning.go:208: return "", fmt.Errorf("findProviderId:
> machine-%010d key not found: %q", m.Id(), PROVIDER_MACHINE_ID)
> i think 'machine %d' would work better - the %010 stuff is detail
> internal to state.
>
> https://codereview.appspot.com/6250068/diff/2001/cmd/jujud/provisioning.go#newcode210
> cmd/jujud/provisioning.go:210: if _, ok := id.(string); !ok {
> rather than doing the type conversion twice, perhaps:
>
> if id, ok := id.(string); ok {
> return id, nil
> }
> return "", fmt.Errorf("machine %d has invalid value for %s: %#v",
> m.Id(), PROVIDER_MACHINE_ID, id)
>
> https://codereview.appspot.com/6250068/diff/2001/cmd/jujud/provisioning.go#newcode221
> cmd/jujud/provisioning.go:221: if len(insts) < 1 {
> this can't happen.
>
> https://codereview.appspot.com/6250068/
>
> --
> https://code.launchpad.net/~dave-cheney/juju/go-provisioning-strawman/+merge/107714
> You are the owner of lp:~dave-cheney/juju/go-provisioning-strawman.