Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force the charmhub naming for URLs #319

Merged
merged 2 commits into from Sep 9, 2020

Conversation

SimonRichardson
Copy link
Member

The following forces non-prefix charm URLs to fallback to charmhub
schemas. This is a much bigger breaking change than the previous
changes. This states that anything withouth a prefix is a charmhub,
where previously that was stated as a charmstore charm URL.

Additionally we drop the prefix of a charm URL during the string
representation of a URL. To get the previous same logic, calling the
following, URL#FullPath() will do it.

This has been tricky as we want to preserve as much of the old way as
possible, but we also don't want to keep things around for no reason.

The following forces non-prefix charm URLs to fallback to charmhub
schemas. This is a much bigger breaking change than the previous
changes. This states that anything withouth a prefix is a charmhub,
where previously that was stated as a charmstore charm URL.

Additionally we drop the prefix of a charm URL during the string
representation of a URL. To get the previous same logic, calling the
following, URL#FullPath() will do it.

This has been tricky as we want to preserve as much of the old way as
possible, but we also don't want to keep things around for no reason.
} else {
parts = append(parts, r.Name)
parts = append(parts, u.Name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious as to how the store deals with URLs that have variable arguments... Does it pattern match on ~ for user and %s-%d for revision?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the charmstore uses the same code as this, or at least it did! Then it parsed it to extrapolate that same information that we had. So in a sense, the URL would be isomorphic between juju and the charmstore. As we never pass the URL directly to the charmhub any longer, we are abusing the URL to just store the information we require, namely <schema>:<name>-<revision>. It has valid useful information in the URL and more importantly, it keeps Juju happy so that we don't have to re-write all the workers that watch for the URL to change.

The longer term, this should be renamed to something other than URL, but that would be a mountain of work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really? Guessing the meaning of args when parsing URLs is just a disaster waiting to happen... Any idea why these are not encoded as query string parameters in the first place?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You see that would have been sane!

<schema>://<username>@<name>?<revision>=&<series>=

url.go Outdated Show resolved Hide resolved
url.go Outdated Show resolved Hide resolved
@SimonRichardson
Copy link
Member Author

$$merge$$

@jujubot jujubot merged commit b1139b8 into juju:master Sep 9, 2020
@SimonRichardson SimonRichardson deleted the force-charmhub-names branch September 10, 2020 13:40
jujubot added a commit to juju/charmrepo that referenced this pull request Nov 18, 2020
#164

juju/os v1 is pulled in as an indirect dependency now due to the charmrepo tests pulling in charmstore.v5 which pulls in juju/os v1.

The `cs:` test fixes are because of the change juju/charm#319 which was actually a backwards-incompatible change to how parsing charm URLs worked -- we no longer support the `~` prefix or `foo/bar` forms with scheme-less URLs.

This change is in aid of getting juju/juju to only use juju/os/v2. See juju/juju#12329
jujubot added a commit to juju/bundlechanges that referenced this pull request Nov 18, 2020
#73

Also fix tests now that we're using the later charm/v8 which doesn't support parsing old charmstore URLs.

The `cs:` test fixes are because of the change juju/charm#319 which was actually a backwards-incompatible change to how parsing charm URLs worked -- we no longer support the `~` prefix or `foo/bar` forms with scheme-less URLs.

This change is in aid of getting juju/juju to only use juju/os/v2. See juju/juju#12329
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants