Code review comment for lp:~jameinel/juju-core/login-returns-env-tag

LGTM

https://codereview.appspot.com/101760046/diff/40001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/101760046/diff/40001/juju/api.go#newcode271
juju/api.go:271: environUUID := ""
s/environUUID/environTag/

https://codereview.appspot.com/101760046/diff/40001/juju/apiconn_test.go
File juju/apiconn_test.go (right):

https://codereview.appspot.com/101760046/diff/40001/juju/apiconn_test.go#newcode283
juju/apiconn_test.go:283: // The local cache doesn't have an EnvironTag,
which the API does
I think this is testing something that is currently impossible. I
suppose it doesn't hurt to be cautious of bugs introduced in the server
though.

https://codereview.appspot.com/101760046/diff/40001/state/api/state_test.go
File state/api/state_test.go (right):

https://codereview.appspot.com/101760046/diff/40001/state/api/state_test.go#newcode105
state/api/state_test.go:105: c.Check(apistate.EnvironTag(), gc.Equals,
env.Tag())
An explicit test for Login with envtag=="" would be good here.

https://codereview.appspot.com/101760046/diff/40001/state/apiserver/admin.go
File state/apiserver/admin.go (right):

https://codereview.appspot.com/101760046/diff/40001/state/apiserver/admin.go#newcode138
state/apiserver/admin.go:138: if c.EnvironTag != "" {
Perhaps add a comment here, saying why we allow an unspecified
EnvironTag, lest someone thinks it's a bug and breaks it?

https://codereview.appspot.com/101760046/diff/40001/state/apiserver/login_test.go
File state/apiserver/login_test.go (right):

https://codereview.appspot.com/101760046/diff/40001/state/apiserver/login_test.go#newcode519
state/apiserver/login_test.go:519:
func (s *loginSuite) TestLoginAcceptsEmptyEnvironTag(c *gc.C) {
...
}

?

https://codereview.appspot.com/101760046/