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 charm path repo implementation #32
Conversation
|
Test PASSed. |
rogpeppe
reviewed
Sep 29, 2015
| + // First try and interpret as a charm URL. | ||
| + if ref, err := charm.ParseReference(curlStr); err == nil { | ||
| + switch ref.Schema { | ||
| + case "cs": |
rogpeppe
Sep 29, 2015
Owner
This will trigger in the case that curlStr=="wordpress" even if there's a wordpress directory. I think you need to check the path first, but I'm thinking that we should have a parameter that specifies whether to do that so that a client has some choice as to whether arbitrary local paths are checked.
Perhaps allowLocalPath bool?
wallyworld
added some commits
Sep 30, 2015
|
Test PASSed. |
|
Test FAILed. |
|
Test PASSed. |
|
Test PASSed. |
rogpeppe
reviewed
Sep 30, 2015
| + | ||
| +// NewBundlePath creates and returns an instance used to | ||
| +// open a bundle located at the given path. | ||
| +func NewBundlePath(path string) (BundlePath, error) { |
rogpeppe
Sep 30, 2015
Owner
Why bother with the BundlePath struct and interface? We could just return (charm.Bundle, *charm.URL, error) from this function and it would be simpler and no less useful, I think.
wallyworld
Sep 30, 2015
Owner
I was thinking we may want to add other methods to the struct later. Juju core doesn't yet deploy local bundles directly so it's hard to know what's required. I thought it would be more consistent to be the same as for charm and allow room for extension later.
rogpeppe
Sep 30, 2015
Owner
I don't see how this approach helps. If you need to extend it, you'll need to change the BundlePath.Bundle method, which will still be a breaking change. If you really wanted the return from NewBundlePath to be extensible it should return a struct to which you could add members, not an interface, but I think it's reasonable to just return what you need to return (the bundle and the URL) directly.
If we need something that returns more, we can always just add another function later and maintain backward compatibility that way.
mhilton
reviewed
Sep 30, 2015
| @@ -0,0 +1 @@ | ||
| +7 |
wallyworld
Sep 30, 2015
Owner
This whole dir was copied from an existing charm and this file was not edited at all.
rogpeppe
reviewed
Sep 30, 2015
| + | ||
| +// NewCharmPath creates and returns an instance used to | ||
| +// open a charm located at the given path. | ||
| +func NewCharmPath(path string) (CharmPath, error) { |
rogpeppe
Sep 30, 2015
Owner
Same here - I can't see what the CharmPath interface adds over just returning (charm.Charm, *charm.URL, error) here.
rogpeppe
Sep 30, 2015
Owner
I'm not convinced that we need the Charm(series) method.
The charm.SeriesForCharm function gets us almost all of the
way there anyway.
Assuming NewCharmPath returns (charm.Charm, *charm.Reference, error),
a client can do:
c, url, err := charmrepo.NewCharmPath(path)
if err != nil {...}
url.Series, err = charm.SeriesForCharm(requestedSeries, c.Meta().Series)
to resolve the series.
I'd suggest that SeriesForCharm returns an error if requestSeries=="" and
there are no series declared in the charm metadata.
I still think that the CharmPath interface is unnecessary.
wallyworld
Oct 1, 2015
Owner
I see where you're coming from but don't particularly like:
url.Series, err = charm.SeriesForCharm(requestedSeries, c.Meta().Series)
as to breaks encapsulation somewhat.
Ok, so what I've done to try and get this accepted is add series to NewCharmAtPath
func NewCharmAtPath(path, series string) (charm.Charm, *charm.URL, error)
It doesn't allow a charm for a series to be peeled off a previously parsed path, but it fits with how core calls this functionality which is simply to get a charm for a given series and then use it.
rogpeppe
reviewed
Sep 30, 2015
| @@ -496,7 +496,7 @@ func (s *charmStoreRepoSuite) TestLatest(c *gc.C) { | ||
| // Run the tests. | ||
| for i, test := range tests { | ||
| c.Logf("test %d: %s", i, test.about) | ||
| - revs, err := s.repo.Latest(test.urls...) | ||
| + revs, err := s.repo.(*charmrepo.CharmStore).Latest(test.urls...) |
rogpeppe
Sep 30, 2015
Owner
Perhaps make charmStoreRepoSuite.repo into a *charmrepo.CharmStore so that we don't need to do this dynamic type coercion?
wallyworld
Sep 30, 2015
Owner
We would still need a type coercion somewhere as NewCharmStore() returns the interface. I'd prefer to do it here and have the rest of the tests work with the interface.
rogpeppe
Sep 30, 2015
Owner
Ah, interesting. Probably NewCharmStore should return *CharmStore not interface then. That would be conventional.
rogpeppe
reviewed
Sep 30, 2015
| @@ -0,0 +1,14 @@ | ||
| +name: bad-charm-with-multi-series | ||
| +summary: "K/V storage engine" | ||
| +description: "Scalable K/V Store in Erlang with Clocks :-)" |
rogpeppe
reviewed
Sep 30, 2015
| - // Latest returns the latest revision of the charms referenced by curls, | ||
| - // regardless of the revision set on each curl. | ||
| - Latest(curls ...*charm.URL) ([]CharmRevision, error) | ||
| - | ||
| // Resolve resolves the series and revision of the given entity | ||
| // reference. If the series is not specified, it may be resolved | ||
| // by the charm store or rejected. After the series is resolved, |
rogpeppe
Sep 30, 2015
Owner
This doesn't sound correct any more. We should clarify the behaviour here when the charm supports more than one series. Under what circumstances is it OK to return an empty list for the possible supported series, for example?
wallyworld
Sep 30, 2015
Owner
Comment adjusted. It's always ok to return an empty list of supported series, if the charm doesn't declare them (old charms).
|
Test PASSed. |
rogpeppe
reviewed
Sep 30, 2015
| + // latest available revision for that entity, possibly accounting | ||
| + // for series if it is specified. | ||
| + // The second return value holds the list of possible series | ||
| + // supported by the entity, with the preferred series first. |
rogpeppe
Sep 30, 2015
Owner
// It may be empty if the charm does not declare any Series in its metadata.
?
Or should that be:
// It will be empty if the charm does not declare any Series in its metadata.
I'm thinking that the latter might be better as it actually provides an indicator
of whether the charm may support multiple series of not.
I'm actually wondering about something like:
// Resolve resolves the given reference to a canonical form which refers
// unambiguously to a specific revision of an entity. If the entity
// is a charm that may support more than one series, canonRef.Series will
// be empty and supportedSeries will hold the list of series supported by
// the charm with the preferred series first.
// If ref holds a series, then Resolve will always ensure that the returned
// entity supports that series.
Resolve(ref *charm.Reference) (canonRef *charm.Reference, supportedSeries []string, err error)
AFAICS there is actually no particular need to change this interface now, as the charm store does not yet support multi-series charms, but I'm OK to do so.
rogpeppe
reviewed
Sep 30, 2015
| } | ||
| // Latest returns the latest revision of the charm referenced by curl, regardless | ||
| // of the revision set on each curl. | ||
| // This is a helper which calls the bulk method and unpacks a single result. | ||
| -func Latest(repo Interface, curl *charm.URL) (int, error) { | ||
| +func Latest(repo *CharmStore, curl *charm.URL) (int, error) { |
rogpeppe
reviewed
Sep 30, 2015
| @@ -0,0 +1,16 @@ | ||
| +name: bad-charm-with-multi-series |
rogpeppe
reviewed
Sep 30, 2015
| } | ||
| - return u.WithRevision(ch.Revision()), nil | ||
| + return u.WithRevision(ch.Revision()), ch.Meta().Series, nil |
rogpeppe
Sep 30, 2015
Owner
I'm not sure that we should encourage the use of charms in series-specific
directories that support multiple series.
I think it's OK to check that ch.Meta().Series is either empty or contains u.Series, but I don't think we should return the supported series here.
For example, I think that a charm that lives under $JUJU_REPOSITORY/precise/wordpress should not be able to support trusty
unless it is moved somewhere else.
rogpeppe
reviewed
Sep 30, 2015
| @@ -0,0 +1,16 @@ | ||
| +name: bad-charm-with-multi-series | ||
| +summary: "K/V storage engine" | ||
| +description: "An example of an charm which exists in a repo under |
|
This is in a reasonable direction, thanks. I have some suggestions for simplification and clarification above. |
|
Test PASSed. |
|
Test PASSed. |
|
Test PASSed. |
rogpeppe
reviewed
Oct 1, 2015
| + "gopkg.in/juju/charm.v6-unstable" | ||
| +) | ||
| + | ||
| +// NewBundleAtPath creates and returns a bundle at a given path. |
rogpeppe
Oct 1, 2015
Owner
// NewBundleAtPath creates and returns a bundle at a given path
// and a URL that describes it.
?
rogpeppe
reviewed
Oct 1, 2015
| + } | ||
| + return nil, nil, err | ||
| + } | ||
| + _, name := filepath.Split(path) |
rogpeppe
Oct 1, 2015
Owner
filepath.Split(filepath.Clean(path)) ?
Otherwise if path is something like "/home/rog/mybundle/", then name will be empty.
wallyworld
Oct 1, 2015
Owner
Done, but I used filepath.Abs() to ensure the path was always expanded correctly so the last dir name could be extracted.
rogpeppe
reviewed
Oct 1, 2015
| +// validated against those the charm declares it supports. | ||
| +func NewCharmAtPath(path, series string) (charm.Charm, *charm.URL, error) { | ||
| + if path == "" { | ||
| + return nil, nil, errgo.New("path to charm not specified") |
rogpeppe
reviewed
Oct 1, 2015
| + "gopkg.in/juju/charm.v6-unstable" | ||
| +) | ||
| + | ||
| +// NewCharmAtPath returns the charm represented by this path. |
rogpeppe
reviewed
Oct 1, 2015
| +) | ||
| + | ||
| +// NewCharmAtPath returns the charm represented by this path. | ||
| +// If the series is not specified, the charm's default |
rogpeppe
reviewed
Oct 1, 2015
| + url := &charm.URL{ | ||
| + Schema: "local", | ||
| + Name: name, | ||
| + Series: seriesToUse, |
rogpeppe
Oct 1, 2015
Owner
I think this can return a URL with an empty series, as SeriesForCharm can return "", nil when meta.Series and series are empty. That breaks the invariant that charm.URL is supposed to maintain. I'd suggest returning *charm.Reference from this method.
As I've mentioned before we should seriously consider getting rid of charm.Reference entirely and relaxing the series-not-empty constraint on charm.URL.
wallyworld
Oct 1, 2015
Owner
SeriesForCharm has been changed probably since you last looked at it to error if it cannot return a non empty series. So URL semantics are still correct.
rogpeppe
reviewed
Oct 1, 2015
| @@ -247,15 +247,11 @@ func (s *CharmStore) Resolve(ref *charm.Reference) (*charm.URL, error) { | ||
| case "": | ||
| etype = "entity" |
rogpeppe
Oct 1, 2015
Owner
I'm thinking we should be consistent with other error messages here and use "charm or bundle". No need to change it in this PR though, just muttering to myself :)
rogpeppe
reviewed
Oct 1, 2015
| + // We return nil for supported series because even though a charm in a local | ||
| + // repository may declare series, it doesn't make sense because charms are | ||
| + // expected to be for a single series only in the repository. The local | ||
| + // repository concept is deprecated for multi series charms. |
|
LGTM thanks, with a few minor suggestions above. |
|
Test PASSed. |
|
Test PASSed. |
|
|
|
Status: merge request accepted. Url: http://ci.jujugui.org:8080/job/charmrepo-merge |
added a commit
that referenced
this pull request
Oct 1, 2015
wallyworld commentedSep 29, 2015
A few changes here: