upgrade-charm now allows upgrading local charms

[kat-co]

This fixes lp:1609463.

This had a few root causes:

1. We naively assumed that arg[0] was the application name. In the case of local charms, this is just the path to a local charm.
2. Even if it is the application name, such checks need to be left to the controller because of potential client/server version mismatches. The controller has the ultimate say on what is considered acceptable.
3. In the event that we try and add a local charm, to verify we're upgrading the same charm, we check whether the application names are the same. We were incorrectly checking the argument to --path as the charm name and not the application name in the metadata.yaml. This was an implicit assumption that the path reflects the application name (not always true).

There are also some very minor drive-by clean-ups in our Sisyphusian effort to converge towards a better codebase.

Further clean-ups in this area are possible; namely:

• Passing in an UpgradeCharmAPI type and using it throughout.
• There is commonality between deploy and upgrade-charm in resolving arguments and doing something useful with them. There is an opportunity to create a component and to pass it into both commands.

[kat-co]

QA Steps:

1.juju deploy ./foo-charm (choose a charm whose path is an invalid application name)
2. juju upgrade-charm foo --path ./foo-charm
3. ???
4. DEPLOYMENT!

[axw]

This is more a review of the assessment, rather than the code.

We naively assumed that arg[0] was the application name. In the case of local charms, this is just the path to a local charm.

It's not naive, unless we're planning to change the behaviour of upgrade-charm. Upgrade-charm is kinda poorly named. It's more like upgrade-application-charm: you specify an application, and we upgrade the application's charm.

We might want to support "juju upgrade-charm <path/to/charm>", but that's only going to work if the application has the same name as the charm. That's not necessarily the case. You can do "juju deploy path/to/charm ".

You can already do "juju upgrade-charm --path <path/to/charm>", which should work. The issue is due to final thing you noted.

Even if it is the application name, such checks need to be left to the controller because of potential client/server version mismatches. The controller has the ultimate say on what is considered acceptable.

We talk in terms of tags, and tags have a specific format that the client and server agree upon. If the user specifies an invalid application name, we cannot form a valid application tag.

In the event that we try and add a local charm, to verify we're upgrading the same charm, we check whether the application names are the same. We were incorrectly checking the argument to --path as the charm name and not the application name in the metadata.yaml. This was an implicit assumption that the path reflects the application name (not always true).

This is the root cause of the issue, IIANM.

[axw]

We talk in terms of tags, and tags have a specific format that the client and server agree upon. If the user specifies an invalid application name, we cannot form a valid application tag.

Of course, the APIs we use there don't use tags. They should, though.

[axw]

As described elsewhere, this really is expected to be an application name. Please revert.

[kat-co]

Hey, sorry, I forgot that this was the case (I even typed out my QA steps wrong...). You are correct that this is the application name, but the check still shouldn't be done client-side (see #2 in commit message)

[kat-co]

Sorry wrote this before coffee. I grok your explanation better, but I don't think my stance has changed. Even if we communicate using what's passed in, I think the controller needs to have the ultimate say.

E.g. (w/ made up versions):

• v1 client, v2 controller, client-side validation: client cannot deploy charms with v2 controller tags.
• v1 client, v2 controller, no client-side validation: client can deploy v2 controller tags and v1 tags if the controller has backwards compatibility.
• v2 client, v1 controller, client-side validation: client cannot deploy charms with v1 controller tags.
• v2 client, v1 controller, no client-side validation: client can deploy v1 controller tags.

[axw]

Can you please move this to a separate PR? It is not required for bug at hand. Then we can debate its merits there.

Most (I hope?) APIs are now using tags, and we do not currently have any tags that differ across API versions AFAIK (except Juju 1 & 2, of course). If we did, that would be an API break, and you would need to bump the facade version. The v2 controller continues to accept v1 tags, while also accepting v2 tags from clients who speak use the new facade version.

[kat-co]

Will do, reverted.

[axw]

LGTM, thanks. If it's not too difficult, please add a test for the path basename != charm name.

[kat-co]

$$merge$$

[jujubot]

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

[jujubot]

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/9475

[kat-co]

$$merge$$

[jujubot]

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju