Split image metadata facade. #7636
Conversation
| @@ -4,7 +4,6 @@ | |||
| package imagemetadata | |||
There was a problem hiding this comment.
the worker is only run by controllers, so it should go under facades/controller
There was a problem hiding this comment.
please make sure the imagemetadata facade allows only controllers to auth
| resources facade.Resources, | ||
| authorizer facade.Authorizer, | ||
| ) (*API, error) { | ||
| if !authorizer.AuthClient() && !authorizer.AuthController() { |
There was a problem hiding this comment.
this facade should allow only clients
if !authorizer.AuthClient() {| type API struct { | ||
| metadata metadataAccess | ||
| newEnviron func() (environs.Environ, error) | ||
| authorizer facade.Authorizer |
There was a problem hiding this comment.
Yes, true now that they are split there is no need. It also means that we no longer need to have ControllerTag call - a lot cleaner code. Done.
| // given filter. | ||
| // Returned list contains metadata ordered by priority. | ||
| func (api *API) List(filter params.ImageMetadataFilter) (params.ListCloudImageMetadataResult, error) { | ||
| if api.authorizer.AuthClient() { |
There was a problem hiding this comment.
the whole facade should only allow clients now, so drop this check, and ones in other non-constructor methods
|
I'd rather add the tests as a follow-up as I wanted to address our current fake authoriser used for tests first - it's really something I wanted to do for a while and will make permission tests easier. Also, it'd be great if these permission tests where done at a higher level now that we have split between client, controller, etc facades. In other words, something along the lines of "test that all facades in facades/client package are client scoped". |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
That would be great, if it's possible. |
This package was pulled out in juju#7631 to create a new controller-side image metadata facade (juju#7636) which was later removed in juju#8400. Since this code is only used by the client facade, we can take it out of common and into the client facade package. Re: tests, only TestSaveModelCfgFailed had to be moved over. The other tests were already covered by existing tests in the client package.
#17394 This PR removes the `ModelConfig` function from `ImageMetadataInterface`, hence removing the dependency on Mongo/state. We have replaced this function with an interface ```go type ModelConfigService interface { ModelConfig(ctx context.Context) (*config.Config, error) } ``` which provides the functionality of the removed function. This interface is implemented by `ServiceFactory.Config()` and mocked out in testing. Now that `ImageMetadataInterface` only provides one method (`Save`), we also renamed it to `ImageMetadataSaver`. I also changed the interface of the `imagecommon.Save` function to take a context and a `ModelConfigService`: ```go func Save( ctx context.Context, service ModelConfigService, saver ImageMetadataSaver, metadata params.MetadataSaveParams, ) ([]params.ErrorResult, error) { ``` Corresponding test changes are included. @nvinuesa also pointed out that the `imagecommon` package was only used by the `client/imagemetadata` facade. It appears this `imagecommon` was originally pulled out (#7631) to create a new controller-side image metadata facade (#7636) which was later removed (#8400). Hence, I have removed this package and pulled all the relevant code into the client package. ### TODO: - [x] fix panicking test - [x] clean up commit history - [x] move imagecommon pkg into imagemetadata **Jira card:** JUJU-6038
Description of change
This PR splits image metadata facade into 2 - one for client and one for the worker.
The worker-related facade remained the original name ImageMetadata and only contains functionality to retrieve and store metadata for published cloud images.
The new client facade, ImageMetadataManager, contains functionality for CRUD operations on image metadata. As these operations are currently only meant to be called as part of the experimental feature with commands sitting behind a feature flag, this facade is also put behind a feature flag.
There are some reservations around whether this facade should be behind a feature flag or not. The debate is around what is more important to us - usability or maintainability. From usability perspective, without the feature flag here, newer clients, that may have these commands exposed, will be able to use functionality without the need to upgrade controllers. However, to be more aligned with the approach on other experimental features, I have put the facade behind a feature flag for now.
The rest of the PR is moving tests and related code around into new locations as well as updating references and import statements.
Note that I have not actually changed the content of the code besides the basic move, rename and typo corrections.
QA steps
There should be no changes to behavior - all unit and CI tests still pass.
Documentation changes
n/a
Bug reference
https://bugs.launchpad.net/juju/+bug/1703582