juju-solutions / libcharmstore

Other
1 stars 6 forks source link

Specify CS endpoint v4 when calling the blues #3

Closed gnuoy closed 7 years ago

gnuoy commented 7 years ago

The OpenStack amulet tests started failing recently. On inspection this appeared to be due to charms being deployed with the wrong series. This seems to be due to a recent commit that removed the api URL from calls to theblues. Previously theblues was called with an api url pointing at the cs v4 endpoint.

Fixes #2

johnsca commented 7 years ago

It would be better to just use the URL provided by theblues. The API version actually supported by theblues is not v4 and there are cases where it will fail when pointed at the v4 API.

johnsca commented 7 years ago

It seems like this PR is actually claiming that the current charm store API version (v5) is broken. If that's the case, I think we should file a bug against the charm store and not force libcharmstore to use an out-dated API version. As I mentioned in my previous comment, there are code paths in theblues that will fail with v4 of the API as they are coded to v5. (I don't recall what they are precisely, but I hit them when working on the Review Queue.)

javacruft commented 7 years ago

This might be todo with which charm store API version is related to which juju version - if v5 is 2.0 oriented then resolving cs:<owner>/<charm> might be correct - so we might need to switch API version depending on which version if juju is being used further up the tool stack.

johnsca commented 7 years ago

@javacruft Neither library (libcharmstore nor theblues) use the Juju client at all, so I don't see how that would be a factor.

From what I can tell, this is actually an issue because the charm in question in multi-series, and the new API drops the series from the URL because it can be used as any of the supported series. The fact that the code using this depends on the series being included in the charm URL thus is no longer valid, even for older Juju versions. However, I could perhaps get behind an argument that if the series is included in the URL in the request, the API should preserve it. Either way, I strongly object to "fixing" this by locking to an old API version rather than fixing either the API behavior or the code depending on it.

javacruft commented 7 years ago

@johnsca I was more referring to juju's use of the charmstore API outside of either libcharmstore or theblues in my previous comment - a quick grok of the codebase:

1.25 -> v4 API (gopkg.in/juju/charmstore.v4) 2.0 -> v5 API (gopkg.in/juju/charmstore.v5-unstable)

I think series Juju 1.25.x is driven by it being encoded in the charm store URL, so charm URL's resolved via the v5 API to URL's without series encoding will drive the wrong behaviour down the stack (as we saw in amulet/juju-deployer/bundletester).

Either way, switching to the new v5 API broke the world looking behind us so we need to find a way forward that supports both use with 1.25.x and with 2.x.

javacruft commented 7 years ago

Infact I remember discussing this with Rick a while back - the v4 API provides a 1.25.x compatibility layer for multi-series charms - which are not understood natively by the older juju version.