Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Bundle deployment base structure. #3249
Conversation
|
QA:
|
rogpeppe
reviewed
Sep 10, 2015
| +// deployBundle deploys the given bundle data using the given API client and | ||
| +// charm store client. The deployment is not transactional, and its progress is | ||
| +// notified using the given command context. | ||
| +func deployBundle(data *charm.BundleData, client *api.Client, csclient *csClient, repoPath string, conf *config.Config, ctx *cmd.Context) error { |
rogpeppe
Sep 10, 2015
Owner
I wonder whether it might be better to provide notifications through an interface rather than directly to a cmd.Context - that way this function is potentially usable server-side too.
rogpeppe
reviewed
Sep 10, 2015
| +// notified using the given command context. | ||
| +func deployBundle(data *charm.BundleData, client *api.Client, csclient *csClient, repoPath string, conf *config.Config, ctx *cmd.Context) error { | ||
| + // TODO frankban: provide a verifyConstraints function. | ||
| + if err := data.Verify(nil); err != nil { |
rogpeppe
Sep 10, 2015
Owner
or just:
data.Verify(func(s string) error {
_, err := constraints.Parse(s)
return err
})
rogpeppe
reviewed
Sep 10, 2015
| @@ -112,19 +112,19 @@ func environFromNameProductionFunc( | ||
| return env, cleanup, err | ||
| } | ||
| -// resolveCharmURL resolves the given charm URL string | ||
| +// resolveEntityURL resolves the given charm or bundle URL string |
rogpeppe
Sep 10, 2015
Owner
There's an unfortunate conflict of naming between the charm store and juju-core. In the charm store, an "entity" refers to a charm or a bundle, but in juju-core, it refers to something with a Tag method (a unit, machine, user etc).
Although this method name would be fine in charmstore context, I suspect it will be confusing when read in the context of juju-core. I'm thinking that instead of "entity" we should use "charmOrBundle" instead. Alternatively "charmStoreEntity" might work (which has the advantage that it will still be accurate if we add another kind of thing to the charm store).
What do you think?
frankban
Sep 10, 2015
Member
I agree, this is a good point. resolveCharmStoreEntityURL seems good to me, renamed.
rogpeppe
reviewed
Sep 10, 2015
| @@ -148,24 +148,24 @@ func resolveCharmURL(curlStr string, csParams charmrepo.NewCharmStoreParams, rep | ||
| // The URL is already fully resolved; do not | ||
| // bother with an unnecessary round-trip to the | ||
| // charm store. | ||
| - curl, err := ref.URL("") | ||
| + url, err := ref.URL("") |
rogpeppe
Sep 10, 2015
Owner
I don't mind it called curl actually. It means that it doesn't alias with the net/url package. It's still a charm.URL (and there are other instances of the curl name in this package too).
rogpeppe
reviewed
Sep 10, 2015
| if err != nil { | ||
| return nil, nil, errors.Trace(err) | ||
| } | ||
| - return curl, repo, nil | ||
| + return url, repo, nil | ||
| } | ||
| // addCharmViaAPI calls the appropriate client API calls to add the | ||
| // given charm URL to state. For non-public charm URLs, this function also | ||
| // handles the macaroon authorization process using the given csClient. | ||
| // The resulting charm URL of the added charm is displayed on stdout. |
rogpeppe
reviewed
Sep 10, 2015
| - curl, repo, err := resolveCharmURL(c.CharmName, csClient.params, ctx.AbsPath(c.RepoPath), conf) | ||
| + | ||
| + repoPath := ctx.AbsPath(c.RepoPath) | ||
| + url, repo, err := resolveEntityURL(c.CharmName, csClient.params, repoPath, conf) |
frankban
added some commits
Sep 10, 2015
|
Great stuff, LGTM thanks! |
|
$$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 |
added a commit
that referenced
this pull request
Sep 11, 2015
frankban commentedSep 10, 2015
For now, the system just ads the charms, but local and cs bundles are
supported, and also bundles including non-public charms.
(Review request: http://reviews.vapour.ws/r/2628/)