Code review comment for lp://staging/~dimitern/juju-core/038-upgrade-charm-switch

Revision history for this message
Roger Peppe (rogpeppe) wrote :

good direction, but does not LGTM quite yet - some logic i don't think
is quite right, as outlined below.

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#newcode38
cmd/juju/upgradecharm.go:38: Note that the given charm must be
compatible with the current one.
Perhaps say what "compatible" means in this context in a concise and
easy to understand way (no need to go into all the details, but an
overview would be good so the user knows what they're up against)

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#newcode106
cmd/juju/upgradecharm.go:106: rev, err := repo.Latest(curl)
i think this is wrong in the context of SwitchURL.

i think the Latest and bump-revision logic should only be triggered when
the charm url does not have a revision number specified.

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm_test.go
File cmd/juju/upgradecharm_test.go (right):

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm_test.go#newcode179
cmd/juju/upgradecharm_test.go:179: s.assertLocalRevision(c, 7,
myriakPath)
also test switching from one cs: charm to another?
and with a revision number explicitly specified.

https://codereview.appspot.com/8540050/diff/1/testing/charm.go
File testing/charm.go (right):

https://codereview.appspot.com/8540050/diff/1/testing/charm.go#newcode62
testing/charm.go:62: check(exec.Command("mv", newDst, renamedDst).Run())
os.Rename(newDst, renamedDst) ?

https://codereview.appspot.com/8540050/

« Back to merge proposal