Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Move consume api off application model facade to applicationoffers controller facade #7336
Conversation
wallyworld
added some commits
May 12, 2017
babbageclunk
reviewed
May 29, 2017
Sorry, I had an unsubmitted review because I wanted to check with you about the Consume move. Submitting it now even though some of it's outdated - I'll move anything that's still relevant to a new one.
| ) (*API, error) { | ||
| if !authorizer.AuthClient() { | ||
| return nil, common.ErrPerm | ||
| } | ||
| - dataDir := resources.Get("dataDir").(common.StringResource) |
babbageclunk
May 29, 2017
Member
Won't you still need to get the remote application icon in the future version?
wallyworld
May 29, 2017
Owner
Yeah, it takes URLs as args so needs to go onto a controller facade so the proxy can determine where to redirect the api call based on the location of the offers
| @@ -853,51 +832,52 @@ func (api *API) AddRelation(args params.AddRelation) (params.AddRelationResults, | ||
| endpoints := make([]string, len(args.Endpoints)) | ||
| // We may have a remote application passed in as the endpoint spec. | ||
| // We'll iterate the endpoints to check. | ||
| - isRemote := false | ||
| + //isRemote := false |
babbageclunk
May 29, 2017
Member
This seems weird - surely even if it's a relation to a remote application you need write access to add a relation in this model?
(I realise it's commented out, I just don't think it should be reinstated when the rest of the code is.)
| + | ||
| +// Consume adds remote applications to the model without creating any | ||
| +// relations. | ||
| +func (api *OffersAPI) Consume(args params.ConsumeApplicationArgs) (params.ConsumeApplicationResults, error) { |
babbageclunk
May 29, 2017
Member
Why do we need to move Consume from the Application facade to the ApplicationOffers facade? (Sorry if we've discussed that before.) It seems like if you're changing consume to accept an offer, rather than just a URL, it only needs to interact with its own model, doesn't it? All of the collection of information from the other model happens when building the offer structure, so that's the part that would need to be controller-level.
I'm sure I'm missing something.
| @@ -141,6 +138,20 @@ func (api *BaseAPI) applicationOffersFromModel( | ||
| return results, nil | ||
| } | ||
| +// checkOfferAccess returns the level of access the authenticated user has to the offer, | ||
| +// so long as it is greater than the requested perm. | ||
| +func (api *BaseAPI) checkOfferAccess(backend Backend, offerName string, perm permission.Access) (permission.Access, error) { |
babbageclunk
May 29, 2017
Member
I think this should return (bool, error) - the uses don't ever use the returned access other than checking that it's not permission.NoAccess.
wallyworld
May 29, 2017
•
Edited 1 time
-
wallyworld
May 29, 2017
Owner
-
May 29, 2017
permission.NoAccess is a legitimate enum value for a permission. It's not the same as getting a string value back and not knowing if empty string means something concrete or just missing, which is where we tend to require the use of the additional bool. I'll land now to progress and we can discuss further - easy to add it in the next PR if needed.
babbageclunk
approved these changes
May 29, 2017
I think this looks good, I only have the minor comments on my other review.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$emrge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
wallyworld commentedMay 12, 2017
Description of change
The Consume() API is on the Application facade. The implementation assumes both source and target models are in the same controller. This won't always be the case. The API needs to be split in two:
a. get the details from the source model
b. save the details to the target model
This PR is one of many - it moves the Consume() API to the ApplicationOffers controller facade and implements the part to save the details to the target mode. Stuff that is now broken is commented out.