Merge lp:~jameinel/juju-core/api-named-resources-datadir into lp:~go-bot/juju-core/trunk

[John A Meinel]

state/apiserver/common: StringResource

This adds a new resource type: common.StringResource. It just lets us
register "dataDir" as a Resource, instead of having to give Client
secret insight into attributes of the srvRoot object. That brings us
closer to having all of the Facades all part of a simple Registry
instead of having special cases.

https://codereview.appspot.com/99410043/

state/apiserver/common: StringResource

This adds a new resource type: common.StringResource. It just lets us
register "dataDir" as a Resource, instead of having to give Client
secret insight into attributes of the srvRoot object. That brings us
closer to having all of the Facades all part of a simple Registry
instead of having special cases.

https://codereview.appspot.com/99410043/

[John A Meinel (jameinel)]

Reviewers: mp+220215_code.launchpad.net,

Message:
Please take a look.

Description:
state/apiserver/common: StringResource

This adds a new resource type: common.StringResource. It just lets us
register "dataDir" as a Resource, instead of having to give Client
secret insight into attributes of the srvRoot object. That brings us
closer to having all of the Facades all part of a simple Registry
instead of having special cases.

https://code.launchpad.net/~jameinel/juju-core/api-named-resources-datadir/+merge/220215

Requires:
https://code.launchpad.net/~jameinel/juju-core/api-named-resources/+merge/220208

(do not edit description out of merge proposal)

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

Affected files (+37, -6 lines):
   A [revision details]
   M state/apiserver/client/client.go
   M state/apiserver/client/run.go
   M state/apiserver/common/resource.go
   M state/apiserver/common/resource_test.go
   M state/apiserver/root.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: state/apiserver/root.go
=== modified file 'state/apiserver/root.go'
--- state/apiserver/root.go 2014-05-20 08:17:21 +0000
+++ state/apiserver/root.go 2014-05-20 09:42:11 +0000
@@ -73,7 +73,8 @@
    resources: common.NewResources(),
    entity: entity,
   }
- r.clientAPI.API = client.NewAPI(r.srv.state, r.resources, r,
r.srv.dataDir)
+ r.resources.RegisterNamed("dataDir", common.StringResource(r.srv.dataDir))
+ r.clientAPI.API = client.NewAPI(r.srv.state, r.resources, r)
   return r
  }

Index: state/apiserver/client/client.go
=== modified file 'state/apiserver/client/client.go'
--- state/apiserver/client/client.go 2014-05-13 04:50:10 +0000
+++ state/apiserver/client/client.go 2014-05-20 09:42:11 +0000
@@ -36,7 +36,6 @@
   auth common.Authorizer
   resources *common.Resources
   client *Client
- dataDir string
   // statusSetter provides common methods for updating an entity's
provisioning status.
   statusSetter *common.StatusSetter
  }
@@ -47,12 +46,11 @@
  }

  // NewAPI creates a new instance of the Client API.
-func NewAPI(st *state.State, resources *common.Resources, authorizer
common.Authorizer, datadir string) *API {
+func NewAPI(st *state.State, resources *common.Resources, authorizer
common.Authorizer) *API {
   r := &API{
    state: st,
    auth: authorizer,
    resources: resources,
- dataDir: datadir,
    statusSetter: common.NewStatusSetter(st, common.AuthAlways(true)),
   }
   r.client = &Client{

Index: state/apiserver/client/run.go
=== modified file 'state/apiserver/client/run.go'
--- state/apiserver/client/run.go 2014-04-23 08:50:28 +0000
+++ state/apiserver/client/run.go 2014-05-20 09:42:11 +0000
@@ -14,6 +14,7 @@
   "launchpad.net/juju-core/instance"
   "launchpad.net/juju-core/state"
   "launchpad.net/juju-core/state/api/params"
+ "launchpad.net/juju-core/state/apiserver/common"
   "launchpad.net/juju-cor...

[Dimiter Naydenov (dimitern)]

LGTM

https://codereview.appspot.com/99410043/

[Roger Peppe (rogpeppe)]

FWIW this seems like an odd twisting of the Resources type to me.

The Resources type is there specifically for a connection
to hold resources on behalf of a client so that they can
be cleaned up when the connection goes away. These
changes seem to be turning it into a general registry of arbitrary
stuff that the api server uses to remember things for itself.

I'm sure I'm lacking insight into the eventual destination here,
but it seems to me that having dataDir registered in this
way is more obscure than "giving Client secret insight into attributes of
the srvRoot object" (it's actually registered by newSrvRoot, and
could be made public if required).

Basically, I prefer to see this stuff statically typed when possible.

On 20 May 2014 10:45, John A Meinel <email address hidden> wrote:
> Reviewers: mp+220215_code.launchpad.net,
>
> Message:
> Please take a look.
>
> Description:
> state/apiserver/common: StringResource
>
> This adds a new resource type: common.StringResource. It just lets us
> register "dataDir" as a Resource, instead of having to give Client
> secret insight into attributes of the srvRoot object. That brings us
> closer to having all of the Facades all part of a simple Registry
> instead of having special cases.
>
> https://code.launchpad.net/~jameinel/juju-core/api-named-resources-datadir/+merge/220215
>
> Requires:
> https://code.launchpad.net/~jameinel/juju-core/api-named-resources/+merge/220208
>
> (do not edit description out of merge proposal)
>
>
> Please review this at https://codereview.appspot.com/99410043/
>
> Affected files (+37, -6 lines):
> A [revision details]
> M state/apiserver/client/client.go
> M state/apiserver/client/run.go
> M state/apiserver/common/resource.go
> M state/apiserver/common/resource_test.go
> M state/apiserver/root.go
>
>
> Index: [revision details]
> === added file '[revision details]'
> --- [revision details] 2012-01-01 00:00:00 +0000
> +++ [revision details] 2012-01-01 00:00:00 +0000
> @@ -0,0 +1,2 @@
> +Old revision: <email address hidden>
> +New revision: <email address hidden>
>
> Index: state/apiserver/root.go
> === modified file 'state/apiserver/root.go'
> --- state/apiserver/root.go 2014-05-20 08:17:21 +0000
> +++ state/apiserver/root.go 2014-05-20 09:42:11 +0000
> @@ -73,7 +73,8 @@
> resources: common.NewResources(),
> entity: entity,
> }
> - r.clientAPI.API = client.NewAPI(r.srv.state, r.resources, r,
> r.srv.dataDir)
> + r.resources.RegisterNamed("dataDir", common.StringResource(r.srv.dataDir))
> + r.clientAPI.API = client.NewAPI(r.srv.state, r.resources, r)
> return r
> }
>
>
>
> Index: state/apiserver/client/client.go
> === modified file 'state/apiserver/client/client.go'
> --- state/apiserver/client/client.go 2014-05-13 04:50:10 +0000
> +++ state/apiserver/client/client.go 2014-05-20 09:42:11 +0000
> @@ -36,7 +36,6 @@
> auth common.Authorizer
> resources *common.Resources
> client *Client
> - dataDir string
> // statusSetter provides common methods for updat...

=== modified file 'state/apiserver/client/client.go'
--- state/apiserver/client/client.go	2014-05-13 04:50:10 +0000
+++ state/apiserver/client/client.go	2014-05-20 09:49:27 +0000
@@ -36,7 +36,6 @@
 	auth      common.Authorizer
 	resources *common.Resources
 	client    *Client
-	dataDir   string
 	// statusSetter provides common methods for updating an entity's provisioning status.
 	statusSetter *common.StatusSetter
 }
@@ -47,12 +46,11 @@
 }
 
 // NewAPI creates a new instance of the Client API.
-func NewAPI(st *state.State, resources *common.Resources, authorizer common.Authorizer, datadir string) *API {
+func NewAPI(st *state.State, resources *common.Resources, authorizer common.Authorizer) *API {
 	r := &API{
 		state:        st,
 		auth:         authorizer,
 		resources:    resources,
-		dataDir:      datadir,
 		statusSetter: common.NewStatusSetter(st, common.AuthAlways(true)),
 	}
 	r.client = &Client{

=== modified file 'state/apiserver/client/run.go'
--- state/apiserver/client/run.go	2014-04-23 08:50:28 +0000
+++ state/apiserver/client/run.go	2014-05-20 09:49:27 +0000
@@ -14,6 +14,7 @@
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api/params"
+	"launchpad.net/juju-core/state/apiserver/common"
 	"launchpad.net/juju-core/utils"
 	"launchpad.net/juju-core/utils/set"
 	"launchpad.net/juju-core/utils/ssh"
@@ -75,6 +76,14 @@
 	return result, nil
 }
 
+func (c *Client) getDataDir() string {
+	dataResource, ok := c.api.resources.Get("dataDir").(common.StringResource)
+	if !ok {
+		return ""
+	}
+	return dataResource.String()
+}
+
 // Run the commands specified on the machines identified through the
 // list of machines, units and services.
 func (c *Client) Run(run params.RunParams) (results params.RunResults, err error) {
@@ -110,7 +119,7 @@
 		execParam := remoteParamsForMachine(machine, command, run.Timeout)
 		params = append(params, execParam)
 	}
-	return ParallelExecute(c.api.dataDir, params), nil
+	return ParallelExecute(c.getDataDir(), params), nil
 }
 
 // RunOnAllMachines attempts to run the specified command on all the machines.
@@ -125,7 +134,7 @@
 	for _, machine := range machines {
 		params = append(params, remoteParamsForMachine(machine, command, run.Timeout))
 	}
-	return ParallelExecute(c.api.dataDir, params), nil
+	return ParallelExecute(c.getDataDir(), params), nil
 }
 
 // RemoteExec extends the standard ssh.ExecParams by providing the machine and

=== modified file 'state/apiserver/common/resource.go'
--- state/apiserver/common/resource.go	2014-05-20 09:49:27 +0000
+++ state/apiserver/common/resource.go	2014-05-20 09:49:27 +0000
@@ -111,3 +111,15 @@
 	defer rs.mu.Unlock()
 	return len(rs.resources)
 }
+
+// StringResource is just a regular 'string' that matches the Resource
+// interface.
+type StringResource string
+
+func (StringResource) Stop() error {
+	return nil
+}
+
+func (s StringResource) String() string {
+	return string(s)
+}

=== modified file 'state/apiserver/common/resource_test.go'
--- state/apiserver/common/resource_test.go	2014-05-20 09:49:27 +0000
+++ state/apiserver/common/resource_test.go	2014-05-20 09:49:27 +0000
@@ -147,3 +147,12 @@
 
 	c.Assert(rs.Count(), gc.Equals, 0)
 }
+
+func (resourceSuite) TestStringResource(c *gc.C) {
+	rs := common.NewResources()
+	r1 := common.StringResource("foobar")
+	id := rs.Register(r1)
+	c.Check(rs.Get(id), gc.Equals, r1)
+	asStr := rs.Get(id).(common.StringResource).String()
+	c.Check(asStr, gc.Equals, "foobar")
+}

=== modified file 'state/apiserver/root.go'
--- state/apiserver/root.go	2014-05-20 09:49:27 +0000
+++ state/apiserver/root.go	2014-05-20 09:49:27 +0000
@@ -73,7 +73,8 @@
 		resources: common.NewResources(),
 		entity:    entity,
 	}
-	r.clientAPI.API = client.NewAPI(r.srv.state, r.resources, r, r.srv.dataDir)
+	r.resources.RegisterNamed("dataDir", common.StringResource(r.srv.dataDir))
+	r.clientAPI.API = client.NewAPI(r.srv.state, r.resources, r)
 	return r
 }