Move the GetBundleChanges call in a new Bundle facade. #6354
Conversation
Also make the new facade available on both controller and model connections. This is done so that GUI users can render the bundle uncommitted state even without a model connection. Note that, even if this is a backward incompatible API change, I think we can be quite sure that the GUI is the only client of this API.
| func (s *serverSuite) TestGetBundleChangesBundleContentError(c *gc.C) { | ||
| args := params.GetBundleChangesParams{ | ||
| type bundleSuite struct { | ||
| jujutesting.JujuConnSuite |
There was a problem hiding this comment.
❌ This doesn't appear to actually need to be a jujuConnSuite... newFacade doesn't actually use s.State, for example. We try to avoid making new suites that depend on JujuConnSuite because it's so slow.
There was a problem hiding this comment.
The StateSuite from state/testing is sufficient if you need a *state.State. See apiserver/pinger_test.go for an example.
| // Licensed under the AGPLv3, see LICENCE file for details. | ||
|
|
||
| package client_test | ||
| package bundle_test |
There was a problem hiding this comment.
❌ please don't use export_test... if the only thing that it is needed is to access an newFacade, just put this test in package bundle, and use the unexported function directly.
There was a problem hiding this comment.
Ok, I ended up exposing NewFacade, which feels right for other reasons as well.
| // GetBundleChanges returns the list of changes required to deploy the given | ||
| // bundle data. The changes are sorted by requirements, so that they can be | ||
| // applied in order. | ||
| func (c *Client) GetBundleChanges(args params.GetBundleChangesParams) (params.GetBundleChangesResults, error) { |
There was a problem hiding this comment.
You can't just delete this from the client facade now. We have committed to backwards compatibility with RC1.
This means that this method needs to stay, but can forward to the new facade for implementation.
| return params.GetBundleChangesResults{}, err | ||
| // init registers the Bundle facade. | ||
| func init() { | ||
| common.RegisterStandardFacade("Bundle", 0, newFacade) |
There was a problem hiding this comment.
All new facades start from version 1, not zero.
| func (s *serverSuite) TestGetBundleChangesBundleContentError(c *gc.C) { | ||
| args := params.GetBundleChangesParams{ | ||
| type bundleSuite struct { | ||
| jujutesting.JujuConnSuite |
There was a problem hiding this comment.
The StateSuite from state/testing is sufficient if you need a *state.State. See apiserver/pinger_test.go for an example.
| // Licensed under the AGPLv3, see LICENCE file for details. | ||
|
|
||
| package client_test |
There was a problem hiding this comment.
You will need to keep a test in the client to show that client calls still work.
| // GetBundleChangesParams holds parameters for making GetBundleChanges calls. | ||
| type GetBundleChangesParams struct { | ||
| // BundleGetChangesParams holds parameters for making Bundle.GetChanges calls. | ||
| type BundleGetChangesParams struct { |
There was a problem hiding this comment.
Why keep the "Get" in the name? If you are changing the name anyway, probably better to remove the "Get" altogether.
BundelChangesParam, and BundleChangesResults.
While you are at it, BundleChangesChange -> BundleChange ?
| @@ -32,5 +37,5 @@ func controllerFacadesOnly(facadeName, _ string) error { | |||
| func isControllerFacade(facadeName string) bool { | |||
| // Note: the Pinger facade can be used in both model and controller | |||
There was a problem hiding this comment.
You should update the Note to match the behaviour.
frankban
force-pushed
the
bundle-facade
branch
2 times, most recently
from
c989c8f
to
024b732
Compare
frankban
force-pushed
the
bundle-facade
branch
from
024b732
to
66e69fd
Compare
| } | ||
|
|
||
| // NewFacade creates and returns a new Bundle API facade. | ||
| func NewFacade(_ *state.State, _ facade.Resources, auth facade.Authorizer) (Bundle, error) { |
There was a problem hiding this comment.
(Sorry for the somewhat conflicting advice. This is a newish thing.)
Please have the RegisterStandardFacade call an unexported function which has this signature, and have that call NewFacade. NewFacade should not mention state.State. In other facades we are creating a "Backend" interface which contains the subset of State that that facade needs. In this case you don't even need State, so you can just not include it in the NewFacade signature.
i.e. something like:
common.RegisterStandardFacade("Bundle", 1, newFacade)
...
func newFacade(_ *state.State, _ facade.Resources, auth facade.Authorizer) (Bundle, error) {
return NewFacade(auth)
}
...
func NewFacade(auth facade.Authorizer) (Bundle, error) {
...
| ) | ||
|
|
||
| type bundleSuite struct { | ||
| statetesting.StateSuite |
There was a problem hiding this comment.
this will be unnecessary if you drop State from the external interface
| model, err := s.State.ControllerModel() | ||
| c.Assert(err, jc.ErrorIsNil) | ||
| auth := apiservertesting.FakeAuthorizer{ | ||
| Tag: model.Owner(), |
There was a problem hiding this comment.
Just set it to some fake user tag. We don't care about the relationship to the model (clearly, since you're not using State), just that it's a client making the request.
|
Thank you for the reviews! |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
Also make the new facade available on both controller and model connections.
This is done so that GUI users can render the bundle uncommitted state even without a model connection.
Note that, even if this is a backward incompatible API change, I think we can be quite sure that the GUI is the only client of this API.