Merge lp:~thumper/juju-core/common-api-env-watcher into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Refactor client side api to have common behaviour

The apiserver components have a common packge that
enables other end points to share the implementation
of some common facilities.

The api client side should also have this common
behaviour.

This branch starts this with the environ watcher
and getter for the provisioner. This will be used
by the Uniter in the next branch.

In order to reduce import cycles the common.Caller
interface had to be moved. It was either that or to
have the common code in a different package. I
decided that it was better to have the same package
name for the common code on both the client and
server side for the api, so common.Caller becomes
base.Caller.

https://codereview.appspot.com/52610045/

Refactor client side api to have common behaviour

The apiserver components have a common packge that
enables other end points to share the implementation
of some common facilities.

The api client side should also have this common
behaviour.

This branch starts this with the environ watcher
and getter for the provisioner. This will be used
by the Uniter in the next branch.

In order to reduce import cycles the common.Caller
interface had to be moved. It was either that or to
have the common code in a different package. I
decided that it was better to have the same package
name for the common code on both the client and
server side for the api, so common.Caller becomes
base.Caller.

https://codereview.appspot.com/52610045/

[Tim Penhey (thumper)]

Reviewers: mp+202766_code.launchpad.net,

Message:
Please take a look.

Description:
Refactor client side api to have common behaviour

The apiserver components have a common packge that
enables other end points to share the implementation
of some common facilities.

The api client side should also have this common
behaviour.

This branch starts this with the environ watcher
and getter for the provisioner. This will be used
by the Uniter in the next branch.

In order to reduce import cycles the common.Caller
interface had to be moved. It was either that or to
have the common code in a different package. I
decided that it was better to have the same package
name for the common code on both the client and
server side for the api, so common.Caller becomes
base.Caller.

https://code.launchpad.net/~thumper/juju-core/common-api-env-watcher/+merge/202766

(do not edit description out of merge proposal)

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

Affected files (+104, -75 lines):
   A [revision details]
   M state/api/agent/state.go
   M state/api/base/caller.go
   M state/api/charmrevisionupdater/updater.go
   A state/api/common/environwatcher.go
   M state/api/deployer/deployer.go
   M state/api/keyupdater/authorisedkeys.go
   M state/api/logger/logger.go
   M state/api/machiner/machiner.go
   M state/api/provisioner/provisioner.go
   M state/api/uniter/uniter.go
   M state/api/upgrader/upgrader.go
   M state/api/watcher/watcher.go

[Ian Booth (wallyworld)]

LGTM with tweaks

https://codereview.appspot.com/52610045/diff/1/state/api/common/environwatcher.go
File state/api/common/environwatcher.go (right):

https://codereview.appspot.com/52610045/diff/1/state/api/common/environwatcher.go#newcode13
state/api/common/environwatcher.go:13: type EnvironWatcher struct {
doc comment

https://codereview.appspot.com/52610045/diff/1/state/api/common/environwatcher.go#newcode14
state/api/common/environwatcher.go:14: name string
perhaps facadeName?

https://codereview.appspot.com/52610045/diff/1/state/api/common/environwatcher.go#newcode18
state/api/common/environwatcher.go:18: func NewEnvironWatcher(name
string, caller base.Caller) *EnvironWatcher {
ditto

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

https://codereview.appspot.com/52610045/diff/1/state/api/provisioner/provisioner.go#newcode16
state/api/provisioner/provisioner.go:16: const Provisioner =
"Provisioner"
lower case please, no need to export

https://codereview.appspot.com/52610045/

[Go Bot (go-bot)]

Attempt to merge into lp:juju-core failed due to conflicts:

text conflict in state/api/deployer/deployer.go
text conflict in state/api/machiner/machiner.go
text conflict in state/api/provisioner/provisioner.go

[Go Bot (go-bot)]

The attempt to merge lp:~thumper/juju-core/common-api-env-watcher into lp:juju-core failed. Below is the output from the failed tests.

# launchpad.net/juju-core/state/api/deployer
state/api/deployer/deployer.go:24: undefined: common
state/api/deployer/deployer.go:24: not enough arguments to return
# launchpad.net/juju-core/state/api/common
state/api/common/life.go:14: undefined: Caller
# launchpad.net/juju-core/state/api/machiner
state/api/machiner/machiner.go:23: undefined: common
state/api/machiner/machiner.go:23: not enough arguments to return
# launchpad.net/juju-core/state/api/uniter
state/api/uniter/uniter.go:29: undefined: common
state/api/uniter/uniter.go:29: not enough arguments to return

=== modified file 'state/api/agent/state.go'
--- state/api/agent/state.go	2013-10-01 10:00:26 +0000
+++ state/api/agent/state.go	2014-01-23 00:09:43 +0000
@@ -7,18 +7,18 @@
 	"fmt"
 
 	"launchpad.net/juju-core/instance"
-	"launchpad.net/juju-core/state/api/common"
+	"launchpad.net/juju-core/state/api/base"
 	"launchpad.net/juju-core/state/api/params"
 )
 
 // State provides access to an agent's view of the state.
 type State struct {
-	caller common.Caller
+	caller base.Caller
 }
 
 // NewState returns a version of the state that provides functionality
 // required by agent code.
-func NewState(caller common.Caller) *State {
+func NewState(caller base.Caller) *State {
 	return &State{caller}
 }
 

=== added directory 'state/api/base'
=== renamed file 'state/api/common/interfaces.go' => 'state/api/base/caller.go'
--- state/api/common/interfaces.go	2013-06-06 09:56:00 +0000
+++ state/api/base/caller.go	2014-01-23 00:09:43 +0000
@@ -1,7 +1,7 @@
 // Copyright 2012, 2013 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
 
-package common
+package base
 
 // Caller is implemented by the client-facing State object.
 type Caller interface {

=== modified file 'state/api/charmrevisionupdater/updater.go'
--- state/api/charmrevisionupdater/updater.go	2014-01-15 07:23:28 +0000
+++ state/api/charmrevisionupdater/updater.go	2014-01-23 00:09:43 +0000
@@ -4,17 +4,17 @@
 package charmrevisionupdater
 
 import (
-	"launchpad.net/juju-core/state/api/common"
+	"launchpad.net/juju-core/state/api/base"
 	"launchpad.net/juju-core/state/api/params"
 )
 
 // State provides access to a worker's view of the state.
 type State struct {
-	caller common.Caller
+	caller base.Caller
 }
 
 // NewState returns a version of the state that provides functionality required by the worker.
-func NewState(caller common.Caller) *State {
+func NewState(caller base.Caller) *State {
 	return &State{caller}
 }
 

=== added file 'state/api/common/environwatcher.go'
--- state/api/common/environwatcher.go	1970-01-01 00:00:00 +0000
+++ state/api/common/environwatcher.go	2014-01-23 00:09:43 +0000
@@ -0,0 +1,56 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package common
+
+import (
+	"launchpad.net/juju-core/environs/config"
+	"launchpad.net/juju-core/state/api/base"
+	"launchpad.net/juju-core/state/api/params"
+	"launchpad.net/juju-core/state/api/watcher"
+)
+
+// EnvironWatcher provides common client side api functions
+// to call into the apiserver.common.EnvironWatcher.
+type EnvironWatcher struct {
+	façadeName string
+	caller     base.Caller
+}
+
+// NewEnvironWatcher creates a EnvironWatcher on the specified façade,
+// and uses this name when calling through the caller.
+func NewEnvironWatcher(façadeName string, caller base.Caller) *EnvironWatcher {
+	return &EnvironWatcher{façadeName, caller}
+}
+
+// WatchForEnvironConfigChanges return a NotifyWatcher waiting for the
+// environment configuration to change.
+func (e *EnvironWatcher) WatchForEnvironConfigChanges() (watcher.NotifyWatcher, error) {
+	var result params.NotifyWatchResult
+	err := e.caller.Call(e.façadeName, "", "WatchForEnvironConfigChanges", nil, &result)
+	if err != nil {
+		return nil, err
+	}
+	if err := result.Error; err != nil {
+		return nil, result.Error
+	}
+	w := watcher.NewNotifyWatcher(e.caller, result)
+	return w, nil
+}
+
+// EnvironConfig returns the current environment configuration.
+func (e *EnvironWatcher) EnvironConfig() (*config.Config, error) {
+	var result params.EnvironConfigResult
+	err := e.caller.Call(e.façadeName, "", "EnvironConfig", nil, &result)
+	if err != nil {
+		return nil, err
+	}
+	if err := result.Error; err != nil {
+		return nil, err
+	}
+	conf, err := config.New(config.NoDefaults, result.Config)
+	if err != nil {
+		return nil, err
+	}
+	return conf, nil
+}

=== modified file 'state/api/common/life.go'
--- state/api/common/life.go	2014-01-22 22:36:32 +0000
+++ state/api/common/life.go	2014-01-23 00:09:43 +0000
@@ -6,12 +6,13 @@
 import (
 	"fmt"
 
+	"launchpad.net/juju-core/state/api/base"
 	"launchpad.net/juju-core/state/api/params"
 )
 
 // Life requests the life cycle of the given entity from the given
 // server-side API facade via the given caller.
-func Life(caller Caller, facadeName, tag string) (params.Life, error) {
+func Life(caller base.Caller, facadeName, tag string) (params.Life, error) {
 	var result params.LifeResults
 	args := params.Entities{
 		Entities: []params.Entity{{Tag: tag}},

=== modified file 'state/api/deployer/deployer.go'
--- state/api/deployer/deployer.go	2014-01-22 22:36:32 +0000
+++ state/api/deployer/deployer.go	2014-01-23 00:09:43 +0000
@@ -4,18 +4,19 @@
 package deployer
 
 import (
+	"launchpad.net/juju-core/state/api/base"
 	"launchpad.net/juju-core/state/api/common"
 	"launchpad.net/juju-core/state/api/params"
 )
 
 // State provides access to the deployer worker's idea of the state.
 type State struct {
-	caller common.Caller
+	caller base.Caller
 }
 
 // NewState creates a new State instance that makes API calls
 // through the given caller.
-func NewState(caller common.Caller) *State {
+func NewState(caller base.Caller) *State {
 	return &State{caller}
 }
 

=== modified file 'state/api/firewaller/firewaller.go'
--- state/api/firewaller/firewaller.go	2014-01-22 22:36:32 +0000
+++ state/api/firewaller/firewaller.go	2014-01-23 00:09:43 +0000
@@ -5,6 +5,7 @@
 
 import (
 	"launchpad.net/juju-core/environs/config"
+	"launchpad.net/juju-core/state/api/base"
 	"launchpad.net/juju-core/state/api/common"
 	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/state/api/watcher"
@@ -12,11 +13,11 @@
 
 // State provides access to the Firewaller API facade.
 type State struct {
-	caller common.Caller
+	caller base.Caller
 }
 
 // NewState creates a new client-side Firewaller facade.
-func NewState(caller common.Caller) *State {
+func NewState(caller base.Caller) *State {
 	return &State{caller}
 }
 

=== modified file 'state/api/keyupdater/authorisedkeys.go'
--- state/api/keyupdater/authorisedkeys.go	2013-12-09 11:40:27 +0000
+++ state/api/keyupdater/authorisedkeys.go	2014-01-23 00:09:43 +0000
@@ -6,18 +6,18 @@
 import (
 	"fmt"
 
-	"launchpad.net/juju-core/state/api/common"
+	"launchpad.net/juju-core/state/api/base"
 	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/state/api/watcher"
 )
 
 // State provides access to a worker's view of the state.
 type State struct {
-	caller common.Caller
+	caller base.Caller
 }
 
 // NewState returns a version of the state that provides functionality required by the worker.
-func NewState(caller common.Caller) *State {
+func NewState(caller base.Caller) *State {
 	return &State{caller}
 }
 

=== modified file 'state/api/logger/logger.go'
--- state/api/logger/logger.go	2013-09-13 04:27:17 +0000
+++ state/api/logger/logger.go	2014-01-23 00:09:43 +0000
@@ -6,19 +6,19 @@
 import (
 	"fmt"
 
-	"launchpad.net/juju-core/state/api/common"
+	"launchpad.net/juju-core/state/api/base"
 	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/state/api/watcher"
 )
 
 // State provides access to an logger worker's view of the state.
 type State struct {
-	caller common.Caller
+	caller base.Caller
 }
 
 // NewState returns a version of the state that provides functionality
 // required by the logger worker.
-func NewState(caller common.Caller) *State {
+func NewState(caller base.Caller) *State {
 	return &State{caller}
 }
 

=== modified file 'state/api/machiner/machiner.go'
--- state/api/machiner/machiner.go	2014-01-22 22:36:32 +0000
+++ state/api/machiner/machiner.go	2014-01-23 00:09:43 +0000
@@ -4,17 +4,18 @@
 package machiner
 
 import (
+	"launchpad.net/juju-core/state/api/base"
 	"launchpad.net/juju-core/state/api/common"
 	"launchpad.net/juju-core/state/api/params"
 )
 
 // State provides access to the Machiner API facade.
 type State struct {
-	caller common.Caller
+	caller base.Caller
 }
 
 // NewState creates a new client-side Machiner facade.
-func NewState(caller common.Caller) *State {
+func NewState(caller base.Caller) *State {
 	return &State{caller}
 }
 

=== modified file 'state/api/provisioner/provisioner.go'
--- state/api/provisioner/provisioner.go	2014-01-22 22:36:32 +0000
+++ state/api/provisioner/provisioner.go	2014-01-23 00:09:43 +0000
@@ -6,26 +6,33 @@
 import (
 	"fmt"
 
-	"launchpad.net/juju-core/environs/config"
+	"launchpad.net/juju-core/state/api/base"
 	"launchpad.net/juju-core/state/api/common"
 	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/state/api/watcher"
 	"launchpad.net/juju-core/tools"
 )
 
+const provisioner = "Provisioner"
+
 // State provides access to the Machiner API facade.
 type State struct {
-	caller common.Caller
+	*common.EnvironWatcher
+
+	caller base.Caller
 }
 
 // NewState creates a new client-side Machiner facade.
-func NewState(caller common.Caller) *State {
-	return &State{caller}
+func NewState(caller base.Caller) *State {
+	return &State{
+		EnvironWatcher: common.NewEnvironWatcher(provisioner, caller),
+
+		caller: caller}
 }
 
 // machineLife requests the lifecycle of the given machine from the server.
 func (st *State) machineLife(tag string) (params.Life, error) {
-	return common.Life(st.caller, "Provisioner", tag)
+	return common.Life(st.caller, provisioner, tag)
 }
 
 // Machine provides access to methods of a state.Machine through the facade.
@@ -41,44 +48,12 @@
 	}, nil
 }
 
-// WatchForEnvironConfigChanges return a NotifyWatcher waiting for the
-// environment configuration to change.
-func (st *State) WatchForEnvironConfigChanges() (watcher.NotifyWatcher, error) {
-	var result params.NotifyWatchResult
-	err := st.caller.Call("Provisioner", "", "WatchForEnvironConfigChanges", nil, &result)
-	if err != nil {
-		return nil, err
-	}
-	if err := result.Error; err != nil {
-		return nil, result.Error
-	}
-	w := watcher.NewNotifyWatcher(st.caller, result)
-	return w, nil
-}
-
-// EnvironConfig returns the current environment configuration.
-func (st *State) EnvironConfig() (*config.Config, error) {
-	var result params.EnvironConfigResult
-	err := st.caller.Call("Provisioner", "", "EnvironConfig", nil, &result)
-	if err != nil {
-		return nil, err
-	}
-	if err := result.Error; err != nil {
-		return nil, err
-	}
-	conf, err := config.New(config.NoDefaults, result.Config)
-	if err != nil {
-		return nil, err
-	}
-	return conf, nil
-}
-
 // WatchEnvironMachines returns a StringsWatcher that notifies of
 // changes to the lifecycles of the machines (but not containers) in
 // the current environment.
 func (st *State) WatchEnvironMachines() (watcher.StringsWatcher, error) {
 	var result params.StringsWatchResult
-	err := st.caller.Call("Provisioner", "", "WatchEnvironMachines", nil, &result)
+	err := st.caller.Call(provisioner, "", "WatchEnvironMachines", nil, &result)
 	if err != nil {
 		return nil, err
 	}
@@ -92,7 +67,7 @@
 // StateAddresses returns the list of addresses used to connect to the state.
 func (st *State) StateAddresses() ([]string, error) {
 	var result params.StringsResult
-	err := st.caller.Call("Provisioner", "", "StateAddresses", nil, &result)
+	err := st.caller.Call(provisioner, "", "StateAddresses", nil, &result)
 	if err != nil {
 		return nil, err
 	}
@@ -102,7 +77,7 @@
 // APIAddresses returns the list of addresses used to connect to the API.
 func (st *State) APIAddresses() ([]string, error) {
 	var result params.StringsResult
-	err := st.caller.Call("Provisioner", "", "APIAddresses", nil, &result)
+	err := st.caller.Call(provisioner, "", "APIAddresses", nil, &result)
 	if err != nil {
 		return nil, err
 	}
@@ -112,7 +87,7 @@
 // CACert returns the certificate used to validate the state connection.
 func (st *State) CACert() ([]byte, error) {
 	var result params.BytesResult
-	err := st.caller.Call("Provisioner", "", "CACert", nil, &result)
+	err := st.caller.Call(provisioner, "", "CACert", nil, &result)
 	if err != nil {
 		return nil, err
 	}
@@ -125,7 +100,7 @@
 	args := params.Entities{
 		Entities: []params.Entity{{Tag: tag}},
 	}
-	err := st.caller.Call("Provisioner", "", "Tools", args, &results)
+	err := st.caller.Call(provisioner, "", "Tools", args, &results)
 	if err != nil {
 		// TODO: Not directly tested
 		return nil, err
@@ -144,6 +119,6 @@
 // ContainerConfig returns information from the environment config that are
 // needed for container cloud-init.
 func (st *State) ContainerConfig() (result params.ContainerConfig, err error) {
-	err = st.caller.Call("Provisioner", "", "ContainerConfig", nil, &result)
+	err = st.caller.Call(provisioner, "", "ContainerConfig", nil, &result)
 	return result, err
 }

=== modified file 'state/api/uniter/uniter.go'
--- state/api/uniter/uniter.go	2014-01-22 22:36:32 +0000
+++ state/api/uniter/uniter.go	2014-01-23 00:09:43 +0000
@@ -8,19 +8,20 @@
 
 	"launchpad.net/juju-core/charm"
 	"launchpad.net/juju-core/names"
+	"launchpad.net/juju-core/state/api/base"
 	"launchpad.net/juju-core/state/api/common"
 	"launchpad.net/juju-core/state/api/params"
 )
 
 // State provides access to the Uniter API facade.
 type State struct {
-	caller common.Caller
+	caller base.Caller
 	// unitTag contains the authenticated unit's tag.
 	unitTag string
 }
 
 // NewState creates a new client-side Uniter facade.
-func NewState(caller common.Caller, authTag string) *State {
+func NewState(caller base.Caller, authTag string) *State {
 	return &State{caller, authTag}
 }
 

=== modified file 'state/api/upgrader/upgrader.go'
--- state/api/upgrader/upgrader.go	2013-10-03 12:18:41 +0000
+++ state/api/upgrader/upgrader.go	2014-01-23 00:09:43 +0000
@@ -6,7 +6,7 @@
 import (
 	"fmt"
 
-	"launchpad.net/juju-core/state/api/common"
+	"launchpad.net/juju-core/state/api/base"
 	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/state/api/watcher"
 	"launchpad.net/juju-core/tools"
@@ -15,12 +15,12 @@
 
 // State provides access to an upgrader worker's view of the state.
 type State struct {
-	caller common.Caller
+	caller base.Caller
 }
 
 // NewState returns a version of the state that provides functionality
 // required by the upgrader worker.
-func NewState(caller common.Caller) *State {
+func NewState(caller base.Caller) *State {
 	return &State{caller}
 }
 

=== modified file 'state/api/watcher/watcher.go'
--- state/api/watcher/watcher.go	2013-12-05 03:42:25 +0000
+++ state/api/watcher/watcher.go	2014-01-23 00:09:43 +0000
@@ -9,7 +9,7 @@
 	"launchpad.net/tomb"
 
 	"launchpad.net/juju-core/log"
-	"launchpad.net/juju-core/state/api/common"
+	"launchpad.net/juju-core/state/api/base"
 	"launchpad.net/juju-core/state/api/params"
 )
 
@@ -114,14 +114,14 @@
 // It does not send content for those changes.
 type notifyWatcher struct {
 	commonWatcher
-	caller          common.Caller
+	caller          base.Caller
 	notifyWatcherId string
 	out             chan struct{}
 }
 
 // If an API call returns a NotifyWatchResult, you can use this to turn it into
 // a local Watcher.
-func NewNotifyWatcher(caller common.Caller, result params.NotifyWatchResult) NotifyWatcher {
+func NewNotifyWatcher(caller base.Caller, result params.NotifyWatchResult) NotifyWatcher {
 	w := &notifyWatcher{
 		caller:          caller,
 		notifyWatcherId: result.NotifyWatcherId,
@@ -172,12 +172,12 @@
 // The content of the changes is a list of strings.
 type stringsWatcher struct {
 	commonWatcher
-	caller           common.Caller
+	caller           base.Caller
 	stringsWatcherId string
 	out              chan []string
 }
 
-func NewStringsWatcher(caller common.Caller, result params.StringsWatchResult) StringsWatcher {
+func NewStringsWatcher(caller base.Caller, result params.StringsWatchResult) StringsWatcher {
 	w := &stringsWatcher{
 		caller:           caller,
 		stringsWatcherId: result.StringsWatcherId,
@@ -230,12 +230,12 @@
 // those units known to have entered.
 type relationUnitsWatcher struct {
 	commonWatcher
-	caller                 common.Caller
+	caller                 base.Caller
 	relationUnitsWatcherId string
 	out                    chan params.RelationUnitsChange
 }
 
-func NewRelationUnitsWatcher(caller common.Caller, result params.RelationUnitsWatchResult) RelationUnitsWatcher {
+func NewRelationUnitsWatcher(caller base.Caller, result params.RelationUnitsWatchResult) RelationUnitsWatcher {
 	w := &relationUnitsWatcher{
 		caller:                 caller,
 		relationUnitsWatcherId: result.RelationUnitsWatcherId,