Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Facade registration cleanup #7133
Conversation
mjs
changed the title from
Apiserver facade cleanup
to
Facade registration cleanup
Mar 22, 2017
| ) | ||
| + | ||
| +// AllFacades returns a registry containing all known API facades. | ||
| +func AllFacades() *facade.Registry { |
wallyworld
Mar 22, 2017
Owner
Thinking out loud - convention in tests at least would be to name this function MustAllFacades()
(as it panics instead of returning an error)
mjs
Mar 22, 2017
Contributor
I'd prefer the name as is, given that there's no alternative. As a compromise, how about I mention the possibility of a panic in the docstring?
| + reg := func(name string, version int, newFunc interface{}) { | ||
| + err := registry.RegisterStandard(name, version, newFunc) | ||
| + if err != nil { | ||
| + panic(err) |
wallyworld
Mar 22, 2017
Owner
can you add a comment that there's a test to ensure we don't panic in production
| @@ -71,7 +58,7 @@ func DeployApplication(backend Backend, args jjj.DeployApplicationParams) error | ||
| return err | ||
| } | ||
| -func newAPI(ctx facade.Context) (*API, error) { | ||
| +func NewFacade(ctx facade.Context) (*API, error) { |
| -// newFacade wraps the supplied *state.State for the use of the Facade. | ||
| -func newFacade(st *state.State, res facade.Resources, auth facade.Authorizer) (*Facade, error) { | ||
| +// NewFacade wraps the supplied *state.State for the use of the Facade. | ||
| +func NewAPI(st *state.State, res facade.Resources, auth facade.Authorizer) (*Facade, error) { |
| @@ -148,3 +149,118 @@ func (f *Registry) Discard(name string, version int) { | ||
| } | ||
| } | ||
| } | ||
| + | ||
| +type niceFactory func(Context) (interface{}, error) |
| @@ -11,7 +11,7 @@ import ( | ||
| ) | ||
| // newFacade wraps New to express the supplied *state.State as a Backend. | ||
| -func newFacade(st *state.State, res facade.Resources, auth facade.Authorizer) (*Facade, error) { | ||
| +func NewFacade(st *state.State, res facade.Resources, auth facade.Authorizer) (*Facade, error) { |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
mjs
added some commits
Mar 3, 2017
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
mjs commentedMar 22, 2017
•
Edited 1 time
-
mjs
Mar 22, 2017
Description of change
This change makes a number of improvements to the way API facades are registered. Highlights:
New facades now just require a call be added to
apiserver.AllFacades.A follow-up PR will tidy up the groupings of facades in
AllFacades.QA steps
Unit tests pass.
Sanity check:
Documentation changes
N.A.
Bug reference
N.A.