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 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-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
>
>
> Index: state/apiserver/common/resource.go
> === modified file 'state/apiserver/common/resource.go'
> --- state/apiserver/common/resource.go  2014-05-20 06:20:06 +0000
> +++ state/apiserver/common/resource.go  2014-05-20 09:42:11 +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)
> +}
>
>
> Index: state/apiserver/common/resource_test.go
> === modified file 'state/apiserver/common/resource_test.go'
> --- state/apiserver/common/resource_test.go     2014-05-20 06:20:06 +0000
> +++ state/apiserver/common/resource_test.go     2014-05-20 09:42:11 +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")
> +}
>
>
>
>
>
> --
> https://code.launchpad.net/~jameinel/juju-core/api-named-resources-datadir/+merge/220215
> Your team juju hackers is requested to review the proposed merge of lp:~jameinel/juju-core/api-named-resources-datadir into lp:juju-core.