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

Revision history for this message
Roger Peppe (rogpeppe) 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/

« Back to merge proposal