Fix bundle deploy idempotency

[johnsca]

When a bundle is deployed which contains an application which already exists in the model, the CLI will skip adding the application but libjuju will raise an error. This fixes the latter behavior.

[SimonRichardson]

Officially, this is the wrong way to do this. Juju should allow you to pass the local constructed model or use the existing switched model and input that into the bundle facade to get the bundle changes diff.
Except that's not how the bundle facade works. Instead, it just gives you a set of changes from the existing bundle, which will always be everything.

The issue here is that we're missing a lot of the logic about how we handle and deploy an application.

The short to medium-term goal is to move bundle changes to be server-side only and the client will never have to worry about this, we're not at that stage though.

My question is, how much do you need this, and at the very least can we add a comment about this being a short-term fix for the deficiencies of Juju?

[johnsca]

@SimonRichardson I agree that the controller should handle the logic of generating the minimal change plan necessary to update the model to match the bundle and that that logic should be used by the CLI client as well. Honestly, I thought the bundle change API already did handle that and was surprised when it failed. I added the suggested comments, as well as adding another case for relations that I missed, to the PR.

I need this for manually verifying some test changes for a couple of PRs which depend on each other. They will naturally unblock themselves once all the PRs land, so I wouldn't say it's critical, but I was surprised by the behavior so figured it would be better to propose this both to unblock myself and in case it could be a workaround until the API can be updated. Correct me if I'm wrong, but if this is fixed in the controller at some point, wouldn't that just make these checks superfluous without being harmful? If that's true, and you're ok with the changes as they are, it seems worth adding for the short term at least.

[johnsca]

Oh, it is worth noting that there definitely are still edge cases that aren't handled properly. For instance, you end up with extra machines allocated each time you run this (although this might be due to having an explicit machines section in my test bundle). I can continue to iterate on this if we think it's worth getting something in place, but I'm also ok with saying that it's not worth putting the effort in here and rather focus on the controller side instead.

[SimonRichardson]

I'm happy with this change, we can always move this to the correct location in the future.

[SimonRichardson]

$$merge$$