Fwd: series-agnostic charm URLs

[roger peppe]

[re-cc'ing to juju-dev]

On 25 July 2014 00:59, Casey Marshall <casey.marshall at canonical.com> wrote:
> charm.URL and charm.Reference helps clearly delineate whether a series
> must be defined using the type system. This is stronger and more robust
> than conditional checking on the series when servicing API requests.

Expressing things in the type system is always a trade off. In this case,
on the one hand, you get some stronger assurance that the series
is actually specified (although not *that* strong, given that the fields
are exported). On the other hand, you make it harder to create
code that's reusable across both kinds of URL.

My view is that having a single "unit of currency" for charm URLs
adds more value than we lose through using a slightly more dynamic
type.

But given that both you and William seem to lean the other way,
I have a slightly different suggestion which would address our needs.

I suggest that we keep both the URL and the Reference types,
but we change Reference to contain exactly the same fields as URL.

We would then avoid the slightly strange anomaly
that we have now where ParseReference doesn't just
parse a reference without a series - the string can *optionally*
contain a series, but that cannot be stored in the Reference.
A Reference would now encapsulate the "sloppy" form of a
URL, and would parse/unmarshal accordingly.

The code would look something like this:

    package charm

    // URL represents a fully resolved charm location with a specific
series, such
    // as:
    //
    //     cs:~joe/oneiric/wordpress
    //     cs:oneiric/wordpress-42
    //     local:oneiric/wordpress
    type URL struct {
        Schema   string // "cs" or "local"
        User     string // "joe"
        Name     string // "wordpress"
        Revision int    // -1 if unset, N otherwise
        Series string
    }

    // Reference represents a charm location with a series
    // that may be unresolved.
    //
    //     cs:~joe/wordpress
    //     cs:wordpress-42
    //     cs:precise/wordpress
    type Reference URL

    // URL returns a full URL from the reference, filling out
    // the series from defaultSeries if necessary.
    func (ref *Reference) URL(defaultSeries string) (*URL, error) {
        if ref.Series == "" {
            if defaultSeries == "" {
                return nil, fmt.Errorf("cannot infer charm URL for %q:
no series provided", ref)
            }
            ref.Series = defaultSeries
        }
        return (*charm.URL)(ref), nil
    }

    // ParseURL parses the provided charm URL string into its respective
    // structure.
    func ParseURL(url string) (*URL, error)

    // ParseReference returns a charm reference inferred from src. The provided
    // src may be a valid URL or it may be an alias in one of the
following formats:
    //
    //    name
    //    name-revision
    //    series/name
    //    series/name-revision
    //    schema:name
    //    schema:name-revision
    //    cs:~user/name
    //    cs:~user/name-revision
    //
    // A missing schema is assumed to be 'cs'.
    func ParseReference(src string) (*Reference, error) {

    // InferURL parses src as a reference, fills out
    // the series in the returned URL using defaultSeries
    // if necessary. This function is deprecated.
    func InferURL(src string, defaultSeries string) (*URL, error) {
        ref, err := ParseReference(src)
        if err != nil {
            return nil, err
        }
        return ref.URL(defaultSeries)
    }

We could actually delete InferURL entirely - there are only
three calls to it in the entire code base, including tests.

> I recommend inventing a new URL type for this new concern, that does
> precisely what you need, without mixing it up with the existing usage in
> core.

I hope we don't need to add a third type here.

  cheers,
    rog.