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 new preferred charm/bundle url format #221
Conversation
babbageclunk
added some commits
Sep 16, 2016
|
Testing done: deploying charms and bundles with v1, v2, and v3 urls, including the ambiguous series/name vs name/series ones. Also tested leaving out optional fields like user, series and revision. |
|
I was worried about suppressing the "bundle" series value for bundle urls in String() if they get stored - we wouldn't be able to reread them from the db unless we resolved them from the charmstore again. I think that's actually fine - we don't store bundle urls in the database, they're implemented client-side. |
mjs
approved these changes
Sep 20, 2016
Nice and tidy - thanks.
I'm a bit concerned about the reliance on the known set of series but can't think of a practical alternative.
| @@ -41,17 +47,19 @@ type URL struct { | ||
| Series string // "precise" or "" if unset; "bundle" if it's a bundle. | ||
| } | ||
| +const bundleSeries = "bundle" | ||
| + | ||
| var ErrUnresolvedUrl error = fmt.Errorf("charm or bundle url series is not resolved") |
| var ErrUnresolvedUrl error = fmt.Errorf("charm or bundle url series is not resolved") | ||
| var ( | ||
| - validSeries = regexp.MustCompile("^[a-z]+([a-z0-9]+)?$") | ||
| + validSeries = set.NewStrings(series.SupportedSeries()...).Union(set.NewStrings(bundleSeries)) |
mjs
Sep 20, 2016
•
Edited 1 time
-
mjs
Sep 20, 2016
-
Sep 20, 2016
This is probably ok but introduces the risk of incorrect behaviour when a new Ubuntu series is created. Every time this happens (every 6 months) thing might not work right until Juju is updated and released, or /usr/share/distro-info/ubuntu.csv is updated.
That said, I can't think of a better solution given the constraints, except perhaps having the parsing function return all possible URLs given the input and then querying the charmstore to determine which ones actually exist. That sounds pretty awful though.
babbageclunk
Sep 20, 2016
Member
I see what you mean. Since there's not a backwards-compatibility issue we can do this now and if we come up with a better option before next April we can add that.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-charm |
babbageclunk commentedSep 19, 2016
Fixes http://pad.lv/1584193.
The new format is user/name/series/revision, with all of the fields but
name being optional.
In the code this is called the V3 format. V1 is the
cs:~user/series/name-revision format, and V2 are full jujucharms urls
such as https://jujucharms.com/u/aisrael/mysql-benchmark/trusty/5. The
code still supports the old formats for backwards compatibility.
To distinguish between the V2 url series/name and the V3 url user/name
we need to use a list of series from juju/util/series.
The charmstore API doesn't yet support the url format that the UI does,
so url.Path() returns the url in the V1 format so that we can request charm info.
String() returns the preferred V3 format for displaying to the user.
As discussed in the bug, I've suppressed the "bundle" series in the display
version of the url - it's needed in the structure so the deploy command can
detect bundles and perform different deployment actions.