Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(16340)

Issue 32880043: cmd/juju/destroymachine.go: enable 1.16 compat

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 5 months ago by jameinel
Modified:
10 years, 5 months ago
Reviewers:
mp+196714, dimitern, fwereade
Visibility:
Public.

Description

cmd/juju/destroymachine.go: enable 1.16 compat See bug #1253643 Essentially "Client.DestroyMachines" didn't exist in 1.16.3 so when we changed 1.16.4 to support "juju destroy-machine --force" it broke compatibility with older versions. The code that used to be in state/state.go as State.DestroyMachines was moved into the API implementation. Rather than restoring it (because we don't want it to be on State), we introduce it just next to the compatibility code. That should make it nice and quick to get rid of all the compat stuff when we get to 1.20. I tested this against 1.16.3 and it properly falls back and errors when we expect it to. I'd like to have more unit-tests for it, but I don't see a way to have the test suite's API server fake that a method isn't available. We'll also want to land this patch into trunk, since we need compatibility with 1.16 there also. https://code.launchpad.net/~jameinel/juju-core/1.16-destroy-machine-compat-1253643/+merge/196714 Requires: https://code.launchpad.net/~jameinel/juju-core/1.16-rpc-not-found/+merge/196713 (do not edit description out of merge proposal)

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+60 lines, -3 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/destroymachine.go View 3 chunks +58 lines, -3 lines 0 comments Download

Messages

Total messages: 3
jameinel
Please take a look.
10 years, 5 months ago (2013-11-26 13:45:29 UTC) #1
dimitern
LGTM
10 years, 5 months ago (2013-11-26 13:50:54 UTC) #2
fwereade
10 years, 5 months ago (2013-11-26 16:07:11 UTC) #3
LGTM, I think this is a good model.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b