state/apiserver/common: factor out Resources

state/apiserver/common: factor out Resources

Given that the type itself provides all the introspection
facilities we need, there seems little point in having an
interface and making a fake version when we can use
the real thing.

https://code.launchpad.net/~rogpeppe/juju-core/335-apiserver-common-resources/+merge/171623

(do not edit description out of merge proposal)

[rog, 2013-06-26 18:16:32]

Please take a look.

[jameinel, 2013-06-26 18:26:03]

LGTM

You lose tracking the Id and the map the Resource is in, but I don't directly
see why we would actually need it. I don't think srvResource existed because of
introspection, it existed to provide those to fields.

bzr qannotate says that Dimiter wrote the code, but there were a couple of
refactorings in there. It might be worthwhile to see if there was an explicit
plan for the fields before we remove them completely.

[fwereade, 2013-06-26 23:56:17]

Somewhat grudging LGTM -- I'm not sure this either gains or loses us much, and
it moves to increase coupling rather than reduce it, but I'm having a hard time
coming up with a concrete consequence here... so long as the ResourceReistry
stays simple. But I feel like it probably will, so ok :).

https://codereview.appspot.com/10611046/diff/3001/state/apiserver/common/reso...
File state/apiserver/common/resource_test.go (right):

https://codereview.appspot.com/10611046/diff/3001/state/apiserver/common/reso...
state/apiserver/common/resource_test.go:26: c.Assert(id, Equals, "1")
Huh, hadn't previous spotted they were strings. Why's that? JSON expressibility
worries? :)

[rog, 2013-06-27 06:14:33]

On 26 June 2013 19:26,  <john.meinel@canonical.com> wrote:
> LGTM
>
> You lose tracking the Id and the map the Resource is in, but I don't
> directly see why we would actually need it. I don't think srvResource
> existed because of introspection, it existed to provide those to fields.
>
> bzr qannotate says that Dimiter wrote the code, but there were a couple
> of refactorings in there. It might be worthwhile to see if there was an
> explicit plan for the fields before we remove them completely.

I was originally to blame for the code. Those attributes were needed only
because srvResource was originally embedded as an API object directly.
It implemented Stop so it needed to be able to find its
own id and the resources struct so it could remove itself.

[rog, 2013-06-27 06:19:40]

> Somewhat grudging LGTM -- I'm not sure this either gains or loses us
> much, and it moves to increase coupling rather than reduce it, but I'm
> having a hard time coming up with a concrete consequence here... so long
> as the ResourceReistry stays simple. But I feel like it probably will,
> so ok :).

My motivation here was that I wanted more of the features
of the real resource registry in tests and I realised that
actually the fake one was going to look exactly like the real
one, so no point in duplicating the code.

If a particular package *does* want to fake it out, they can
still declare an interface and use that (they can also embed
the real type to get an underlying working implementation
if desired).

https://codereview.appspot.com/10611046/diff/3001/state/apiserver/common/reso...
File state/apiserver/common/resource_test.go (right):

https://codereview.appspot.com/10611046/diff/3001/state/apiserver/common/reso...
state/apiserver/common/resource_test.go:26: c.Assert(id, Equals, "1")
On 2013/06/26 23:56:18, fwereade wrote:
> Huh, hadn't previous spotted they were strings. Why's that? JSON
expressibility
> worries? :)

It's used in the top level rpc method that fetches
a watcher object. That's part of the point of a
Resource.

[rog, 2013-06-27 09:24:12]

Please take a look.