Merge lp:~jameinel/juju-core/api-register-standard-facade into lp:~go-bot/juju-core/trunk
Status: | Work in progress |
---|---|
Proposed branch: | lp:~jameinel/juju-core/api-register-standard-facade |
Merge into: | lp:~go-bot/juju-core/trunk |
Prerequisite: | lp:~jameinel/juju-core/api-facade-registry |
Diff against target: |
250 lines (+209/-0) 3 files modified
state/apiserver/common/export_test.go (+9/-0) state/apiserver/common/registry.go (+85/-0) state/apiserver/common/registry_test.go (+115/-0) |
To merge this branch: | bzr merge lp:~jameinel/juju-core/api-register-standard-facade |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju Engineering | Pending | ||
Review via email: mp+219660@code.launchpad.net |
Description of the change
state/apiserver
So my previous patch introduced a Registry that can be used for
registering and then looking up Facades. However, almost all of our
existing code has methods like:
func NewKeyManagerAPI(
st *state.State,
resources *common.Resources,
authorizer common.Authorizer,
) (*KeyManagerAPI, error) {
And that has the property that it is != o
type FacadeFactory func(
st *state.State, resources *Resources, authorizer Authorizer, id string,
) (interface{}, error)
Because the return type for the latter is interface{} and the former is
a concrete type (that implements that interface, of course).
I could think of 3 ways to work around this, and this seemed the most
tasteful.
1) Rewrite all of the New*API() functions to return interface{}, and
then rewrite all of their tests to cast it back to the concrete type. I
like that New* returns a concrete type, though, so I'd rather keep that.
2) Wrap all of the calls to RegisterFacade with a bespoke func() that
just does the type currying, then all of the init() functions looked
like:
func init() {
common.
st *state.State, resources *Resources, authorizer Authorizer, id string,
) (interface{}, error) {
if id != "" {
return nil, common.ErrBadId
}
return NewKeyManagerAP
})
}
That isn't terrible, but it is a lot of boilerplate to write.
3) Use reflect to create the boilerplate functions. Then all of the
actual init lines just become:
func init() {
common.
}
So at the cost of having RegisterStandar
introspection, it gives us really nice syntax when we want to add more.
(And note that when we have multiple versions of Facades, (2) would have
to have a bespoke func() for each one.)
I could go back to (2) if people don't like the reflect magic, but I
think it does make things nicer overall.
Unmerged revisions
- 2730. By John A Meinel
-
Merged api-facade-registry into api-register-
standard- facade. - 2729. By John A Meinel
-
Merged api-facade-registry into api-register-
standard- facade. - 2728. By John A Meinel
-
merge api-facade-registry to get updated trunk and github.
com/juju/ errors - 2727. By John A Meinel
-
Merged api-facade-registry into api-register-
standard- facade. - 2726. By John A Meinel
-
Merged api-facade-registry into api-register-
standard- facade. - 2725. By John A Meinel
-
merge deletion of ServeCaller
- 2724. By John A Meinel
-
Merge up the changes and resolve the conflicts
- 2723. By John A Meinel
-
merge in just the RegisterStandar
dFacade helpers. - 2722. By John A Meinel
-
merge describe-
api-versions but strip things down to just having the registry. - 2721. By John A Meinel
-
merge in rpc-version.
This gives us all of the functionality for passing Version in the RPC,
and changes the MethodCaller signature so that it can take the version
and the objId.
Reviewers: mp+219660_ code.launchpad. net,
Message:
Please take a look.
Description: /common: RegisterStandar dFacade
state/apiserver
So my previous patch introduced a Registry that can be used for
registering and then looking up Facades. However, almost all of our
existing code has methods like:
func NewKeyManagerAPI(
st *state.State,
resources *common.Resources,
authorizer common.Authorizer,
) (*KeyManagerAPI, error) {
And that has the property that it is != o
type FacadeFactory func(
st *state.State, resources *Resources, authorizer Authorizer, id
string,
) (interface{}, error)
Because the return type for the latter is interface{} and the former is
a concrete type (that implements that interface, of course).
I could think of 3 ways to work around this, and this seemed the most
tasteful.
1) Rewrite all of the New*API() functions to return interface{}, and
then rewrite all of their tests to cast it back to the concrete type. I
like that New* returns a concrete type, though, so I'd rather keep that.
2) Wrap all of the calls to RegisterFacade with a bespoke func() that
just does the type currying, then all of the init() functions looked
like:
func init() { RegisterFacade( "KeyManager" , 0, func( I(st, resources, authorizer), nil
common.
st *state.State, resources *Resources, authorizer Authorizer,
id string,
) (interface{}, error) {
if id != "" {
return nil, common.ErrBadId
}
return NewKeyManagerAP
})
}
That isn't terrible, but it is a lot of boilerplate to write.
3) Use reflect to create the boilerplate functions. Then all of the
actual init lines just become:
func init() {
common. RegisterStandar dFacade( "KeyManager" , 0, NewKeyManagerAPI)
}
So at the cost of having RegisterStandar dFacade have to do reflect
introspection, it gives us really nice syntax when we want to add more.
(And note that when we have multiple versions of Facades, (2) would have
to have a bespoke func() for each one.)
I could go back to (2) if people don't like the reflect magic, but I
think it does make things nicer overall.
https:/ /code.launchpad .net/~jameinel/ juju-core/ api-register- standard- facade/ +merge/ 219660
Requires: /code.launchpad .net/~jameinel/ juju-core/ api-facade- registry/ +merge/ 219654
https:/
(do not edit description out of merge proposal)
Please review this at https:/ /codereview. appspot. com/99290043/
Affected files (+211, -0 lines): /common/ export_ test.go /common/ registry. go /common/ registry_ test.go
A [revision details]
A state/apiserver
M state/apiserver
M state/apiserver