Merge lp:~frankban/juju-core/env-name-in-context into lp:~go-bot/juju-core/trunk

[Francesco Banconi]

Add the environment name to the hooks context.

Expose the human friendly environment name to the
hooks through the JUJU_ENV_NAME environment variable.

This is required e.g. by the GUI in order to give to the user
a hint about where to find the password to log in.

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

Add the environment name to the hooks context.

Expose the human friendly environment name to the
hooks through the JUJU_ENV_NAME environment variable.

This is required e.g. by the GUI in order to give to the user
a hint about where to find the password to log in.

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

[Francesco Banconi (frankban)]

Reviewers: mp+204450_code.launchpad.net,

Message:
Please take a look.

Description:
Add the environment name to the hooks context.

Expose the human friendly environment name to the
hooks through the JUJU_ENV_NAME environment variable.

This is required e.g. by the GUI in order to give to the user
a hint about where to find the password to log in.

https://code.launchpad.net/~frankban/juju-core/env-name-in-context/+merge/204450

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/50090044/

Affected files (+113, -38 lines):
   A [revision details]
   M doc/charms-in-action.txt
   M state/api/params/internal.go
   M state/api/uniter/environ.go
   M state/api/uniter/environ_test.go
   M state/api/uniter/uniter.go
   M state/apiserver/uniter/uniter.go
   M state/apiserver/uniter/uniter_test.go
   M state/environ.go
   M state/environ_test.go
   M worker/uniter/context.go
   M worker/uniter/context_test.go
   M worker/uniter/uniter.go

[Dimiter Naydenov (dimitern)]

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/

[Francesco Banconi (frankban)]

Please take a look.

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.
On 2014/02/03 11:20:12, dimitern wrote:
> // EnvironmentResult holds the result of an API call returning a name
and UUID
> for an environment ?

> Also s/CurrentEnvironmentResult/EnvironmentResult/ ?

Done.

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
On 2014/02/03 11:20:12, dimitern wrote:
> Can you update the bug to exclude Environ.UUID() from the list,
because with
> this change I think we can consider it cached.

Done.

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.
On 2014/02/03 11:20:12, dimitern wrote:
> No need to change this comment I think - see other examples in
api/uniter.

Done.

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 {
On 2014/02/03 11:20:12, dimitern wrote:
> you need to check result.Error as well, in addition to checking err -
see
> APIAddresses below.

Done.

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) {
On 2014/02/03 11:20:12, dimitern wrote:
> // environment1dot16 requests just the UUID of the current
environment, when
> using an older apiserver that does not support CurrentEnvironment API
call. ?

Done.

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

[Francesco Banconi (frankban)]

Thank you!

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

=== modified file 'doc/charms-in-action.txt'
--- doc/charms-in-action.txt	2013-05-27 08:03:26 +0000
+++ doc/charms-in-action.txt	2014-02-03 11:46:53 +0000
@@ -104,6 +104,7 @@
   * $JUJU_CONTEXT_ID and $JUJU_AGENT_SOCKET are set (but should not be messed
     with: the command line tools won't work without them).
   * $JUJU_API_ADDRESSES holds a space separated list of juju API addresses.
+  * $JUJU_ENV_NAME holds the human friendly name of the current environment.
 
 Hook tools
 ----------

=== modified file 'state/api/params/internal.go'
--- state/api/params/internal.go	2014-01-21 18:43:35 +0000
+++ state/api/params/internal.go	2014-02-03 11:46:53 +0000
@@ -96,7 +96,7 @@
 	Results []StringResult
 }
 
-// CharmArchiveURLResult holds a charm archive (bunle) URL, a
+// CharmArchiveURLResult holds a charm archive (bundle) URL, a
 // DisableSSLHostnameVerification flag or an error.
 type CharmArchiveURLResult struct {
 	Error                          *Error
@@ -111,6 +111,14 @@
 	Results []CharmArchiveURLResult
 }
 
+// EnvironmentResult holds the result of an API call returning a name and UUID
+// for an environment.
+type EnvironmentResult struct {
+	Error *Error
+	Name  string
+	UUID  string
+}
+
 // ResolvedModeResult holds a resolved mode or an error.
 type ResolvedModeResult struct {
 	Error *Error

=== modified file 'state/api/uniter/environ.go'
--- state/api/uniter/environ.go	2013-09-06 16:46:48 +0000
+++ state/api/uniter/environ.go	2014-02-03 11:46:53 +0000
@@ -3,30 +3,21 @@
 
 package uniter
 
-import (
-	"launchpad.net/juju-core/state/api/params"
-)
-
 // This module implements a subset of the interface provided by
 // state.Environment, as needed by the uniter API.
 
 // Environment represents the state of an environment.
 type Environment struct {
-	st *State
+	name string
+	uuid string
 }
 
 // UUID returns the universally unique identifier of the environment.
-//
-// NOTE: This differs from state.Environment.UUID() by returning an
-// error as well, because it needs to make an API call
-//
-// TODO(dimitern): 2013-09-06 bug 1221834
-// Cache this after getting it once - it's immutable.
-func (e Environment) UUID() (string, error) {
-	var result params.StringResult
-	err := e.st.caller.Call("Uniter", "", "CurrentEnvironUUID", nil, &result)
-	if err != nil {
-		return "", err
-	}
-	return result.Result, nil
+func (e Environment) UUID() string {
+	return e.uuid
+}
+
+// Name returns the human friendly name of the environment.
+func (e Environment) Name() string {
+	return e.name
 }

=== modified file 'state/api/uniter/environ_test.go'
--- state/api/uniter/environ_test.go	2013-09-06 13:54:56 +0000
+++ state/api/uniter/environ_test.go	2014-02-03 11:46:53 +0000
@@ -5,16 +5,26 @@
 
 import (
 	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/state/api/uniter"
 )
 
 type environSuite struct {
 	uniterSuite
+	apiEnviron   *uniter.Environment
+	stateEnviron *state.Environment
 }
 
 var _ = gc.Suite(&environSuite{})
 
 func (s *environSuite) SetUpTest(c *gc.C) {
 	s.uniterSuite.SetUpTest(c)
+	var err error
+	s.apiEnviron, err = s.uniter.Environment()
+	c.Assert(err, gc.IsNil)
+	s.stateEnviron, err = s.State.Environment()
+	c.Assert(err, gc.IsNil)
 }
 
 func (s *environSuite) TearDownTest(c *gc.C) {
@@ -22,11 +32,9 @@
 }
 
 func (s *environSuite) TestUUID(c *gc.C) {
-	apiEnviron, err := s.uniter.Environment()
-	c.Assert(err, gc.IsNil)
-	stateEnviron, err := s.State.Environment()
-	c.Assert(err, gc.IsNil)
-	uuid, err := apiEnviron.UUID()
-	c.Assert(err, gc.IsNil)
-	c.Assert(uuid, gc.Equals, stateEnviron.UUID())
+	c.Assert(s.apiEnviron.UUID(), gc.Equals, s.stateEnviron.UUID())
+}
+
+func (s *environSuite) TestName(c *gc.C) {
+	c.Assert(s.apiEnviron.Name(), gc.Equals, s.stateEnviron.Name())
 }

=== modified file 'state/api/uniter/uniter.go'
--- state/api/uniter/uniter.go	2014-01-23 00:10:10 +0000
+++ state/api/uniter/uniter.go	2014-02-03 11:46:53 +0000
@@ -155,7 +155,38 @@
 
 // Environment returns the environment entity.
 func (st *State) Environment() (*Environment, error) {
-	return &Environment{st}, nil
+	var result params.EnvironmentResult
+	err := st.caller.Call("Uniter", "", "CurrentEnvironment", nil, &result)
+	if params.IsCodeNotImplemented(err) {
+		// Fall back to using the 1.16 API.
+		return st.environment1dot16()
+	}
+	if err != nil {
+		return nil, err
+	}
+	if err := result.Error; err != nil {
+		return nil, err
+	}
+	return &Environment{
+		name: result.Name,
+		uuid: result.UUID,
+	}, nil
+}
+
+// environment1dot16 requests just the UUID of the current environment, when
+// using an older API server that does not support CurrentEnvironment API call.
+func (st *State) environment1dot16() (*Environment, error) {
+	var result params.StringResult
+	err := st.caller.Call("Uniter", "", "CurrentEnvironUUID", nil, &result)
+	if err != nil {
+		return nil, err
+	}
+	if err := result.Error; err != nil {
+		return nil, err
+	}
+	return &Environment{
+		uuid: result.Result,
+	}, nil
 }
 
 // APIAddresses returns the list of addresses used to connect to the API.

=== modified file 'state/apiserver/uniter/uniter.go'
--- state/apiserver/uniter/uniter.go	2014-01-22 22:06:48 +0000
+++ state/apiserver/uniter/uniter.go	2014-02-03 11:46:53 +0000
@@ -749,6 +749,17 @@
 	return result, err
 }
 
+// CurrentEnvironment returns the name and UUID for the current juju environment.
+func (u *UniterAPI) CurrentEnvironment() (params.EnvironmentResult, error) {
+	result := params.EnvironmentResult{}
+	env, err := u.st.Environment()
+	if err == nil {
+		result.Name = env.Name()
+		result.UUID = env.UUID()
+	}
+	return result, err
+}
+
 // ProviderType returns the provider type used by the current juju
 // environment.
 //

=== modified file 'state/apiserver/uniter/uniter_test.go'
--- state/apiserver/uniter/uniter_test.go	2014-01-24 12:39:48 +0000
+++ state/apiserver/uniter/uniter_test.go	2014-02-03 11:46:53 +0000
@@ -900,6 +900,19 @@
 	c.Assert(result, gc.DeepEquals, params.StringResult{Result: env.UUID()})
 }
 
+func (s *uniterSuite) TestCurrentEnvironment(c *gc.C) {
+	env, err := s.State.Environment()
+	c.Assert(err, gc.IsNil)
+
+	result, err := s.uniter.CurrentEnvironment()
+	c.Assert(err, gc.IsNil)
+	expected := params.EnvironmentResult{
+		Name: env.Name(),
+		UUID: env.UUID(),
+	}
+	c.Assert(result, gc.DeepEquals, expected)
+}
+
 func (s *uniterSuite) addRelation(c *gc.C, first, second string) *state.Relation {
 	eps, err := s.State.InferEndpoints([]string{first, second})
 	c.Assert(err, gc.IsNil)

=== modified file 'state/environ.go'
--- state/environ.go	2014-01-08 02:46:32 +0000
+++ state/environ.go	2014-02-03 11:46:53 +0000
@@ -55,6 +55,11 @@
 	return e.doc.UUID
 }
 
+// Name returns the human friendly name of the environment.
+func (e *Environment) Name() string {
+	return e.doc.Name
+}
+
 // Life returns whether the environment is Alive, Dying or Dead.
 func (e *Environment) Life() Life {
 	return e.doc.Life

=== modified file 'state/environ_test.go'
--- state/environ_test.go	2013-11-19 06:43:27 +0000
+++ state/environ_test.go	2014-02-03 11:46:53 +0000
@@ -28,6 +28,10 @@
 	c.Assert(s.env.Tag(), gc.Equals, expected)
 }
 
+func (s *EnvironSuite) TestName(c *gc.C) {
+	c.Assert(s.env.Name(), gc.Equals, "testenv")
+}
+
 func (s *EnvironSuite) TestUUID(c *gc.C) {
 	uuidA := s.env.UUID()
 	c.Assert(uuidA, gc.HasLen, 36)

=== modified file 'worker/uniter/context.go'
--- worker/uniter/context.go	2014-01-28 04:58:43 +0000
+++ worker/uniter/context.go	2014-02-03 11:46:53 +0000
@@ -60,6 +60,9 @@
 	// uuid is the universally unique identifier of the environment.
 	uuid string
 
+	// envName is the human friendly name of the environment.
+	envName string
+
 	// relationId identifies the relation for which a relation hook is
 	// executing. If it is -1, the context is not running a relation hook;
 	// otherwise, its value must be a valid key into the relations map.
@@ -84,13 +87,14 @@
 	proxySettings osenv.ProxySettings
 }
 
-func NewHookContext(unit *uniter.Unit, id, uuid string, relationId int,
-	remoteUnitName string, relations map[int]*ContextRelation,
+func NewHookContext(unit *uniter.Unit, id, uuid, envName string,
+	relationId int, remoteUnitName string, relations map[int]*ContextRelation,
 	apiAddrs []string, serviceOwner string, proxySettings osenv.ProxySettings) (*HookContext, error) {
 	ctx := &HookContext{
 		unit:           unit,
 		id:             id,
 		uuid:           uuid,
+		envName:        envName,
 		relationId:     relationId,
 		remoteUnitName: remoteUnitName,
 		relations:      relations,
@@ -184,6 +188,7 @@
 		"JUJU_AGENT_SOCKET=" + socketPath,
 		"JUJU_UNIT_NAME=" + ctx.unit.Name(),
 		"JUJU_ENV_UUID=" + ctx.uuid,
+		"JUJU_ENV_NAME=" + ctx.envName,
 		"JUJU_API_ADDRESSES=" + strings.Join(ctx.apiAddrs, " "),
 	}
 	if r, found := ctx.HookRelation(); found {

=== modified file 'worker/uniter/context_test.go'
--- worker/uniter/context_test.go	2014-01-23 21:57:42 +0000
+++ worker/uniter/context_test.go	2014-02-03 11:46:53 +0000
@@ -181,6 +181,7 @@
 		env: map[string]string{
 			"JUJU_UNIT_NAME":     "u/0",
 			"JUJU_API_ADDRESSES": expectedApiAddrs,
+			"JUJU_ENV_NAME":      "test-env-name",
 			"http_proxy":         "http",
 			"HTTP_PROXY":         "http",
 			"https_proxy":        "https",
@@ -195,6 +196,7 @@
 		env: map[string]string{
 			"JUJU_UNIT_NAME":     "u/0",
 			"JUJU_API_ADDRESSES": expectedApiAddrs,
+			"JUJU_ENV_NAME":      "test-env-name",
 			"JUJU_RELATION":      "db",
 			"JUJU_RELATION_ID":   "db:1",
 			"JUJU_REMOTE_UNIT":   "",
@@ -207,6 +209,7 @@
 		env: map[string]string{
 			"JUJU_UNIT_NAME":     "u/0",
 			"JUJU_API_ADDRESSES": expectedApiAddrs,
+			"JUJU_ENV_NAME":      "test-env-name",
 			"JUJU_RELATION":      "db",
 			"JUJU_RELATION_ID":   "db:1",
 			"JUJU_REMOTE_UNIT":   "r/1",
@@ -710,8 +713,9 @@
 		_, found := s.relctxs[relid]
 		c.Assert(found, gc.Equals, true)
 	}
-	context, err := uniter.NewHookContext(s.apiUnit, "TestCtx", uuid, relid, remote,
-		s.relctxs, apiAddrs, "test-owner", proxies)
+	context, err := uniter.NewHookContext(s.apiUnit, "TestCtx", uuid,
+		"test-env-name", relid, remote, s.relctxs, apiAddrs, "test-owner",
+		proxies)
 	c.Assert(err, gc.IsNil)
 	return context
 }
@@ -764,6 +768,7 @@
 		"JUJU_CONTEXT_ID":          "TestCtx",
 		"JUJU_AGENT_SOCKET":        "/path/to/socket",
 		"JUJU_UNIT_NAME":           "u/0",
+		"JUJU_ENV_NAME":            "test-env-name",
 	}
 	for key, value := range expected {
 		c.Check(executionEnvironment[key], gc.Equals, value)

=== modified file 'worker/uniter/uniter.go'
--- worker/uniter/uniter.go	2014-01-28 04:58:43 +0000
+++ worker/uniter/uniter.go	2014-02-03 11:46:53 +0000
@@ -66,6 +66,7 @@
 	relationers   map[int]*Relationer
 	relationHooks chan hook.Info
 	uuid          string
+	envName       string
 
 	dataDir      string
 	baseDir      string
@@ -189,10 +190,8 @@
 	if err != nil {
 		return err
 	}
-	u.uuid, err = env.UUID()
-	if err != nil {
-		return err
-	}
+	u.uuid = env.UUID()
+	u.envName = env.Name()
 
 	runListenerSocketPath := filepath.Join(u.baseDir, RunListenerFile)
 	logger.Debugf("starting juju-run listener on %s:%s", RunListenerNetType, runListenerSocketPath)
@@ -341,8 +340,8 @@
 
 	// Make a copy of the proxy settings.
 	proxySettings := u.proxy
-	return NewHookContext(u.unit, hctxId, u.uuid, relationId, remoteUnitName,
-		ctxRelations, apiAddrs, ownerTag, proxySettings)
+	return NewHookContext(u.unit, hctxId, u.uuid, u.envName, relationId,
+		remoteUnitName, ctxRelations, apiAddrs, ownerTag, proxySettings)
 }
 
 func (u *Uniter) acquireHookLock(message string) (err error) {