Code review comment for lp:~frankban/juju-core/env-name-in-context

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

LGTM with a few suggestions below. Nice!

https://codereview.appspot.com/50090044/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/50090044/diff/1/state/api/params/internal.go#newcode115
state/api/params/internal.go:115: // environment.
// EnvironmentResult holds the result of an API call returning a name
and UUID for an environment ?

Also s/CurrentEnvironmentResult/EnvironmentResult/ ?

https://codereview.appspot.com/50090044/diff/1/state/api/uniter/environ.go
File state/api/uniter/environ.go (left):

https://codereview.appspot.com/50090044/diff/1/state/api/uniter/environ.go#oldcode23
state/api/uniter/environ.go:23: // TODO(dimitern): 2013-09-06 bug
1221834

Can you update the bug to exclude Environ.UUID() from the list, because
with this change I think we can consider it cached.

https://codereview.appspot.com/50090044/diff/1/state/api/uniter/environ.go
File state/api/uniter/environ.go (right):

https://codereview.appspot.com/50090044/diff/1/state/api/uniter/environ.go#newcode9
state/api/uniter/environ.go:9: // Environment represents the name and
the unique identifier of an environment.
No need to change this comment I think - see other examples in
api/uniter.

https://codereview.appspot.com/50090044/diff/1/state/api/uniter/uniter.go
File state/api/uniter/uniter.go (right):

https://codereview.appspot.com/50090044/diff/1/state/api/uniter/uniter.go#newcode164
state/api/uniter/uniter.go:164: if err != nil {
you need to check result.Error as well, in addition to checking err -
see APIAddresses below.

https://codereview.appspot.com/50090044/diff/1/state/api/uniter/uniter.go#newcode173
state/api/uniter/uniter.go:173: func (st *State) environment1dot16()
(*Environment, error) {
// environment1dot16 requests just the UUID of the current environment,
when using an older apiserver that does not support CurrentEnvironment
API call. ?

https://codereview.appspot.com/50090044/

« Back to merge proposal