Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add remote application URL support to reference other controller models directly #6757
Conversation
babbageclunk
approved these changes
Jan 3, 2017
Looks good to me, only a few typos/comments/questions.
(Sorry you had to do it!)
| + if url.ModelName == "" { | ||
| + return nil, errors.Errorf("missing model name in URL %q", url.String()) | ||
| + } | ||
| + // We have an application from another model in the same controller. |
babbageclunk
Jan 3, 2017
Member
Maybe split this section off into a separate method - processSameControllerApplication?
| - remoteApp, err := backend.RemoteApplication(url.ApplicationName) | ||
| +// saveRemoteApplication saves the details of the specified remote application and its endpoints | ||
| +// to the sate model so relations to the remote application can be created. |
| + s.assertAddRelation(c, endpoints) | ||
| +} | ||
| + | ||
| +func (s *serviceSuite) addTestingCharmOtherModel(c *gc.C, name string) *state.Charm { |
wallyworld
Jan 3, 2017
Owner
This helper method operates on the test suite attribute "otherModel", which is created in test setup. So OtherModel in the method title refers to that attribute name.
| @@ -58,7 +58,7 @@ func (s *applicationOffersAPIFactory) Stop() error { | ||
| // ApplicationOffersAPIFactoryResource is a function which returns the application offer api factory | ||
| // which can be used as an facade resource. | ||
| func ApplicationOffersAPIFactoryResource(st *state.State) (facade.Resource, error) { | ||
| - controllerModelStata := st | ||
| + controllerModelState := st |
| func ParseApplicationURL(urlStr string) (*ApplicationURL, error) { | ||
| - url, err := gourl.Parse(urlStr) | ||
| + urlParts, err := parseApplicationURLParts(urlStr, false) |
| - if parts[0] == "u" { | ||
| - if !names.IsValidUser(parts[1]) { | ||
| - return nil, errors.NotValidf("user name %q", parts[1]) | ||
| + // Validate the resulting URL part values. |
babbageclunk
Jan 3, 2017
Member
Should this validation for user/model/application be done for model.application as well, or is this covered completely by the regex matching?
|
$$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 |
wallyworld commentedJan 3, 2017
The remote application url parsing now supports the new syntax for specifying applications in other models on the same controller. The old syntax local:/u/me/model/app is removed.
Applications in another model are now referenced using syntax model.application.
There are now 2 url syntaxes to handle: from an application directory and the new controller model one.
QA:
juju bootstrap
juju deploy mysql
juju switch controller
juju deploy wordpress
juju relate wordpress default.mysql