juju-core

Merge lp:~dimitern/juju-core/038-upgrade-charm-switch into lp:~juju/juju-core/trunk

  1. 038-upgrade-charm-switch
  2. Merge into trunk
Proposed by Dimiter Naydenov
Status: Merged
Merged at revision: 1201
Proposed branch: lp:~dimitern/juju-core/038-upgrade-charm-switch
Merge into: lp:~juju/juju-core/trunk
Diff against target: 332 lines (+180/-37)
3 files modified
cmd/juju/upgradecharm.go (+88/-29)
cmd/juju/upgradecharm_test.go (+83/-8)
testing/charm.go (+9/-0)
To merge this branch: bzr merge lp:~dimitern/juju-core/038-upgrade-charm-switch
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+160910@code.launchpad.net

Description of the change

cmd/juju: upgrade-charm --switch support

This implements a new argument to upgrade-charm:
--switch <charm-url>. The passed URL is inferred
to get the complete URL and used instead of the
service's newest charm url revision.
Also --revision is now supported, to give an
explicit revision to upgrade to, rather than the
latest.

There are a few related sanity checks that need
to be implemented as well (in follow-ups), so we
can better check charm compatibility before upgrading.

Fixes bugs #1040210 and #1050750.

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

To post a comment you must log in.
Revision history for this message
Dimiter Naydenov (dimitern) wrote :
Download full text (7.4 KiB)

Reviewers: mp+160910_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/juju: upgrade-charm --switch support

This implements a new argument to upgrade-charm:
--switch <charm-url>. The passed URL is inferred
to get the complete URL and used instead of the
service's newest charm url revision.

There are a few related sanity checks that need
to be implemented as well (in follow-ups), so we
can better check charm compatibility before upgrading.
Fixes bug #1040203.

https://code.launchpad.net/~dimitern/juju-core/038-upgrade-charm-switch/+merge/160910

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/8540050/

Affected files:
   A [revision details]
   M cmd/juju/upgradecharm.go
   M cmd/juju/upgradecharm_test.go
   M testing/charm.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision:
<email address hidden>
+New revision:
<email address hidden>

Index: testing/charm.go
=== modified file 'testing/charm.go'
--- testing/charm.go 2013-02-11 05:54:45 +0000
+++ testing/charm.go 2013-04-25 14:27:20 +0000
@@ -54,6 +54,15 @@
   return clone(dst, r.DirPath(name))
  }

+// RenamedClonedDirPath returns the path to a new copy of the default
+// charm directory named name, but renames it to newName.
+func (r *Repo) RenamedClonedDirPath(dst, name, newName string) string {
+ newDst := clone(dst, r.DirPath(name))
+ renamedDst := filepath.Join(filepath.Dir(newDst), newName)
+ check(exec.Command("mv", newDst, renamedDst).Run())
+ return renamedDst
+}
+
  // ClonedDir returns an actual charm.Dir based on a new copy of the charm
directory
  // named name, in the directory dst.
  func (r *Repo) ClonedDir(dst, name string) *charm.Dir {

Index: cmd/juju/upgradecharm.go
=== modified file 'cmd/juju/upgradecharm.go'
--- cmd/juju/upgradecharm.go 2013-04-25 10:46:51 +0000
+++ cmd/juju/upgradecharm.go 2013-04-25 14:27:20 +0000
@@ -17,6 +17,7 @@
   ServiceName string
   Force bool
   RepoPath string // defaults to JUJU_REPOSITORY
+ SwitchURL string
  }

  const upgradeCharmDoc = `
@@ -32,6 +33,10 @@
  author working on a single client machine; use of local repositories from
  multiple clients is not supported and may lead to confusing behaviour.

+To manually specify the charm URL to upgrade to, use the --switch argument.
+It will be used instead of the service's current charm newest revision.
+Note that the given charm must be compatible with the current one.
+
  Use of the --force flag is not generally recommended; units upgraded while
in
  an error state will not have upgrade-charm hooks executed, and may cause
  unexpected behavior.
@@ -50,6 +55,7 @@
   c.EnvCommandBase.SetFlags(f)
   f.BoolVar(&c.Force, "force", false, "upgrade all units immediately, even
if in error state")
   f.StringVar(&c.RepoPath, "repository",
os.Getenv("JUJU_REPOSITORY"), "local charm repository path")
+ f.StringVar(&c.SwitchURL, "switch", "", "charm URL to upgrade to")
  }

  func (c *Up...

Read more...

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/

Revision history for this message
William Reade (fwereade) wrote :

a few thoughts

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

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#oldcode67
cmd/juju/upgradecharm.go:67: // TODO(dimitern): add the other flags
--switch and --revision.
I don't see an implementation of --revision, and I think it'd be quite
easy to sneak it into this CL ;)

(consider that --switch and --revision could in theory conflict, so it
might be good to barf here if they're both set)

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.
On 2013/04/25 14:43:52, rog wrote:
> 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)

and underscore that it's dangerous -- we can only check so much
compatibility :)

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#newcode100
cmd/juju/upgradecharm.go:100: curl, _ = service.CharmURL()
wrt rog's comment below, curl = curl.WithRevision(-1)

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#newcode106
cmd/juju/upgradecharm.go:106: rev, err := repo.Latest(curl)
On 2013/04/25 14:43:52, rog wrote:
> 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.

+1

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#newcode94
cmd/juju/upgradecharm_test.go:94: func (s *UpgradeCharmSuccessSuite)
assertUpgraded(c *C, revision int, forced bool, curl *charm.URL) {
I'd rather return the charm url and assert it in the places it's needed,
I think

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

We have thus far gone with the shared agreement that if it works with
one repo (local), we can expect it to work with the other. Hmm, can we
now fake out the charm store? I think we can. If so, definite +1 on
testing this in a wider range of scenarios.

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())
On 2013/04/25 14:43:52, rog wrote:
> os.Rename(newDst, renamedDst) ?

+1

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

Revision history for this message
Dimiter Naydenov (dimitern) wrote :
Download full text (4.2 KiB)

Nothing changed yet, I just need to make sure I get the suggestions
first.

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

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#oldcode67
cmd/juju/upgradecharm.go:67: // TODO(dimitern): add the other flags
--switch and --revision.
On 2013/04/25 15:09:06, fwereade wrote:
> I don't see an implementation of --revision, and I think it'd be quite
 easy to
> sneak it into this CL ;)

> (consider that --switch and --revision could in theory conflict, so it
might be
> good to barf here if they're both set)

So --revision is an int and forces the revision as specified, but only
when --switch is not present, otherwise - barf?

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.
On 2013/04/25 15:09:06, fwereade wrote:
> On 2013/04/25 14:43:52, rog wrote:
> > 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)

> and underscore that it's dangerous -- we can only check so much
compatibility :)

I'd appreciate suggestions with actual text, since probably both of you
can devise a better sentence than me :)

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#newcode100
cmd/juju/upgradecharm.go:100: curl, _ = service.CharmURL()
On 2013/04/25 15:09:06, fwereade wrote:
> wrt rog's comment below, curl = curl.WithRevision(-1)

When --switch is given, the revision shouldn't be bumped, and the code
below does not apply, right?

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#newcode106
cmd/juju/upgradecharm.go:106: rev, err := repo.Latest(curl)
On 2013/04/25 15:09:06, fwereade wrote:
> On 2013/04/25 14:43:52, rog wrote:
> > 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.

> +1

See the question above.

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#newcode94
cmd/juju/upgradecharm_test.go:94: func (s *UpgradeCharmSuccessSuite)
assertUpgraded(c *C, revision int, forced bool, curl *charm.URL) {
On 2013/04/25 15:09:06, fwereade wrote:
> I'd rather return the charm url and assert it in the places it's
 needed, I think

Good point, will do.

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm_test.go#newcode107
cmd/juju/upgradecharm_test.go:107: func (s *UpgradeCharmSuccessSuite)
assertLocalRevision(c *C, revision int, path string) {
On 2013/04/25 14:40:06, TheMue wrote:
> Maybe I'm only blind or it is planned for later, but where are you
 using this
> method with a different path than s.path?

Only in the last t...

Read more...

Revision history for this message
William Reade (fwereade) wrote :

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

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#oldcode67
cmd/juju/upgradecharm.go:67: // TODO(dimitern): add the other flags
--switch and --revision.
On 2013/04/25 15:29:01, dimitern wrote:
> On 2013/04/25 15:09:06, fwereade wrote:
> > I don't see an implementation of --revision, and I think it'd be
 quite easy to
> > sneak it into this CL ;)
> >
> > (consider that --switch and --revision could in theory conflict, so
 it might
> be
> > good to barf here if they're both set)

> So --revision is an int and forces the revision as specified, but only
when
> --switch is not present, otherwise - barf?

Yeah, I think that's sane.

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#newcode100
cmd/juju/upgradecharm.go:100: curl, _ = service.CharmURL()
On 2013/04/25 15:29:01, dimitern wrote:
> On 2013/04/25 15:09:06, fwereade wrote:
> > wrt rog's comment below, curl = curl.WithRevision(-1)

> When --switch is given, the revision shouldn't be bumped, and the code
below
> does not apply, right?

Hmm.I suspect that bump-revision logic *should* apply when --switch is
given with a *local* charm url *without* an explicit revision. Sane?

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#newcode106
cmd/juju/upgradecharm.go:106: rev, err := repo.Latest(curl)
On 2013/04/25 15:29:01, dimitern wrote:
> See the question above.

So I think it's:

rev := curl.Revision
if rev == -1 {
     latest, err := repo.Latest(curl)
     if err != nil {
         return err
     }
     rev = latest
}

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)
On 2013/04/25 15:29:01, dimitern wrote:
> On 2013/04/25 15:09:06, fwereade wrote:
> > On 2013/04/25 14:43:52, rog wrote:
> > > also test switching from one cs: charm to another?
> > > and with a revision number explicitly specified.
> >
> > We have thus far gone with the shared agreement that if it works
 with one repo
> > (local), we can expect it to work with the other. Hmm, can we now
 fake out the
> > charm store? I think we can. If so, definite +1 on testing this in a
 wider
> range
> > of scenarios.

> Sorry, I have no clue how to test this - ideas?

I'd be fine with setting charm.Store to some other repo and checking
that it talks properly to what it *thinks* is the store

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

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

Please take a look.

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.
On 2013/04/25 15:29:01, dimitern wrote:
> On 2013/04/25 15:09:06, fwereade wrote:
> > On 2013/04/25 14:43:52, rog wrote:
> > > 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)
> >
> > and underscore that it's dangerous -- we can only check so much
 compatibility
> :)

> I'd appreciate suggestions with actual text, since probably both of
you can
> devise a better sentence than me :)

Please check the new wording.

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#newcode100
cmd/juju/upgradecharm.go:100: curl, _ = service.CharmURL()
On 2013/04/25 15:43:30, fwereade wrote:
> On 2013/04/25 15:29:01, dimitern wrote:
> > On 2013/04/25 15:09:06, fwereade wrote:
> > > wrt rog's comment below, curl = curl.WithRevision(-1)
> >
> > When --switch is given, the revision shouldn't be bumped, and the
 code below
> > does not apply, right?

> Hmm.I suspect that bump-revision logic *should* apply when --switch is
given
> with a *local* charm url *without* an explicit revision. Sane?

Done.

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#newcode106
cmd/juju/upgradecharm.go:106: rev, err := repo.Latest(curl)
On 2013/04/25 15:43:30, fwereade wrote:
> On 2013/04/25 15:29:01, dimitern wrote:
> > See the question above.

> So I think it's:

> rev := curl.Revision
> if rev == -1 {
> latest, err := repo.Latest(curl)
> if err != nil {
> return err
> }
> rev = latest
> }

Done.

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)
On 2013/04/25 15:43:30, fwereade wrote:
> On 2013/04/25 15:29:01, dimitern wrote:
> > On 2013/04/25 15:09:06, fwereade wrote:
> > > On 2013/04/25 14:43:52, rog wrote:
> > > > also test switching from one cs: charm to another?
> > > > and with a revision number explicitly specified.
> > >
> > > We have thus far gone with the shared agreement that if it works
 with one
> repo
> > > (local), we can expect it to work with the other. Hmm, can we now
 fake out
> the
> > > charm store? I think we can. If so, definite +1 on testing this in
 a wider
> > range
> > > of scenarios.
> >
> > Sorry, I have no clue how to test this - ideas?

> I'd be fine with setting charm.Store to some other repo and checking
that it
> talks properly to what it *thinks* is the store

Not sure how to do that either - it seems I have to spin up a whole http
server for that. If that's what it takes, ok, but it seems an overkill.

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

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

still not quite there i'm afraid.

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

https://codereview.appspot.com/8540050/diff/10001/cmd/juju/upgradecharm.go#newcode46
cmd/juju/upgradecharm.go:46: as the old charm.
probably worth reflowing all this text.

https://codereview.appspot.com/8540050/diff/10001/cmd/juju/upgradecharm.go#newcode51
cmd/juju/upgradecharm.go:51: to upgrade to, rather than the newest one.
This cannot be combined with --switch.
perhaps add:

To specify a given revision number with --switch, give
it in the charm URL, for instance cs:wordpress-5
would specify revision number 5 of the wordpress charm.

?

https://codereview.appspot.com/8540050/diff/10001/cmd/juju/upgradecharm.go#newcode123
cmd/juju/upgradecharm.go:123: latest, err := repo.Latest(curl)
i think this still isn't quite right.
if someone specifies a revision number in the charm url, this means we
then discard it and use the latest revision anyway.

i think fwereade's earlier suggestion is sound ("bump-revision logic
should apply when --switch is given
with a local charm url without an explicit revision")

AFAICS this code doesn't implement that logic.

https://codereview.appspot.com/8540050/diff/10001/cmd/juju/upgradecharm.go#newcode133
cmd/juju/upgradecharm.go:133: // This is not a local repository.
i think that's self evident from the condition on the if statement.

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

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

Please take a look.

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

https://codereview.appspot.com/8540050/diff/10001/cmd/juju/upgradecharm.go#newcode46
cmd/juju/upgradecharm.go:46: as the old charm.
On 2013/04/26 15:06:33, rog wrote:
> probably worth reflowing all this text.

Done.

https://codereview.appspot.com/8540050/diff/10001/cmd/juju/upgradecharm.go#newcode51
cmd/juju/upgradecharm.go:51: to upgrade to, rather than the newest one.
This cannot be combined with --switch.
On 2013/04/26 15:06:33, rog wrote:
> perhaps add:

> To specify a given revision number with --switch, give
> it in the charm URL, for instance cs:wordpress-5
> would specify revision number 5 of the wordpress charm.

> ?

Done.

https://codereview.appspot.com/8540050/diff/10001/cmd/juju/upgradecharm.go#newcode133
cmd/juju/upgradecharm.go:133: // This is not a local repository.
On 2013/04/26 15:06:33, rog wrote:
> i think that's self evident from the condition on the if statement.

Removed.

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

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

Sorry, forgot to reply to these - all done.

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

https://codereview.appspot.com/8540050/diff/10001/cmd/juju/upgradecharm.go#newcode123
cmd/juju/upgradecharm.go:123: latest, err := repo.Latest(curl)
On 2013/04/26 15:06:33, rog wrote:
> i think this still isn't quite right.
> if someone specifies a revision number in the charm url, this means we
 then
> discard it and use the latest revision anyway.

> i think fwereade's earlier suggestion is sound ("bump-revision logic
should
> apply when --switch is given
> with a local charm url without an explicit revision")

> AFAICS this code doesn't implement that logic.

Done.

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

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

Please take a look.

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

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

Please take a look.

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

Revision history for this message
William Reade (fwereade) wrote :

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

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#oldcode66
cmd/juju/upgradecharm.go:66: }
if c.SwitchURL != "" && c.Revision != -1 {
     return fmt.Errorf("--switch and --revision are mutually exclusive")
}

...or something

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#oldcode95
cmd/juju/upgradecharm.go:95: return fmt.Errorf("already running latest
charm %q", curl)
Hmm. If the --force flag is set to something different to before, we
might actually want to allow this case (and other error below)?.
Pre-existing bug, doesn't have to be addressed in this CL, but if it
isn't it should be recorded.

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

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode111
cmd/juju/upgradecharm.go:111: return fmt.Errorf("cannot specify --switch
and --revision together")
This can and should be detected at Init() time.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode113
cmd/juju/upgradecharm.go:113: var err error
Not needed (but read the next comment before addressing it).

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode161
cmd/juju/upgradecharm.go:161: if considerBumpRevision {
I think this can all be simpler.

oldURL, _ := service.CharmURL()
var newURL *charm.URL
if c.SwitchURL != "" {
     // A new charm URL was explicitly specified.
     conf, err := conn.State.EnvironConfig()
     if err != nil {
         return err
     }
     newURL, err = charm.InferURL(c.SwitchURL, conf.DefaultSeries())
     if err != nil {
         return err
     }
} else {
     // No new URL was specified, but a revision might have been.
     newURL = oldURL.WithRevision(c.Revision)
}
repo, err := charm.InferRepository(newURL, ctx.AbsPath(c.RepoPath))
if err != nil {
     return err
}
// If no revision was explicitly specified by either Revision or
// SwitchURL, discover the latest from the repo.
if newURL.Revision == -1 {
     latest, err := repo.Latest(newURL)
     if err != nil {
         return err
     }
     newURL = newURL.WithRevision(latest)
}
if *newURL == *oldURL {
     if _, isLocal := repo.(*charm.LocalRepository); !isLocal {
         // etc

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

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

Please take a look.

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

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#oldcode66
cmd/juju/upgradecharm.go:66: }
On 2013/04/28 10:41:25, fwereade wrote:
> if c.SwitchURL != "" && c.Revision != -1 {
> return fmt.Errorf("--switch and --revision are mutually
 exclusive")
> }

> ...or something

Done.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#oldcode95
cmd/juju/upgradecharm.go:95: return fmt.Errorf("already running latest
charm %q", curl)
On 2013/04/28 10:41:25, fwereade wrote:
> Hmm. If the --force flag is set to something different to before, we
 might
> actually want to allow this case (and other error below)?.
 Pre-existing bug,
> doesn't have to be addressed in this CL, but if it isn't it should be
 recorded.

I added a TODO

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

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode111
cmd/juju/upgradecharm.go:111: return fmt.Errorf("cannot specify --switch
and --revision together")
On 2013/04/28 10:41:25, fwereade wrote:
> This can and should be detected at Init() time.

Done.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode113
cmd/juju/upgradecharm.go:113: var err error
On 2013/04/28 10:41:25, fwereade wrote:
> Not needed (but read the next comment before addressing it).
 See the answer below.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode161
cmd/juju/upgradecharm.go:161: if considerBumpRevision {
On 2013/04/28 10:41:25, fwereade wrote:
> I think this can all be simpler.

> oldURL, _ := service.CharmURL()
> var newURL *charm.URL
> if c.SwitchURL != "" {
> // A new charm URL was explicitly specified.
> conf, err := conn.State.EnvironConfig()
> if err != nil {
> return err
> }
> newURL, err = charm.InferURL(c.SwitchURL, conf.DefaultSeries())
> if err != nil {
> return err
> }
> } else {
> // No new URL was specified, but a revision might have been.
> newURL = oldURL.WithRevision(c.Revision)
> }
> repo, err := charm.InferRepository(newURL, ctx.AbsPath(c.RepoPath))
> if err != nil {
> return err
> }
> // If no revision was explicitly specified by either Revision or
> // SwitchURL, discover the latest from the repo.
> if newURL.Revision == -1 {
> latest, err := repo.Latest(newURL)
> if err != nil {
> return err
> }
> newURL = newURL.WithRevision(latest)
> }
> if *newURL == *oldURL {
> if _, isLocal := repo.(*charm.LocalRepository); !isLocal {
> // etc

I prefer to keep it as is, if you don't mind, and I don't think it's any
more clear the suggested way.
Does it fulfill the required set of cases? I think so.

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

Revision history for this message
William Reade (fwereade) wrote :
Download full text (4.9 KiB)

I'm on the fence a little; let's have a chat about how bad the
revision-bumping situation is, it might be that the benefits of your
implementation will win out.

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

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#oldcode95
cmd/juju/upgradecharm.go:95: return fmt.Errorf("already running latest
charm %q", curl)
On 2013/04/29 09:53:21, dimitern wrote:
> On 2013/04/28 10:41:25, fwereade wrote:
> > Hmm. If the --force flag is set to something different to before, we
 might
> > actually want to allow this case (and other error below)?.
 Pre-existing bug,
> > doesn't have to be addressed in this CL, but if it isn't it should
 be
> recorded.

> I added a TODO

A bug would be great too please.

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

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode27
cmd/juju/upgradecharm.go:27: originally deployed.
  An explicit revision can be chosen with the --revision flag.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode50
cmd/juju/upgradecharm.go:50: The new charm may add new relations and
configuration settings.
How about:

The --switch flag allows you to replace the charm with an entirely
different one. The new charm's URL and revision are inferred as they
would be when running a deploy command.

Please note that --switch is dangerous, because juju only has limited
information with which to determine compatibility; the operation will
succeed, regardless of potential havoc, so long as the following
conditions hold:

- The new charm must declare all relations that the service is currently
participating in.
- All config settings shared by the old and new charms must have the
same types.

--switch and --revision are mutually exclusive.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode56
cmd/juju/upgradecharm.go:56: specify revision number 5 of the wordpress
charm.
Drop this para, --revision is best described at the top, I think; we
just need to mention it won't work with switch.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode76
cmd/juju/upgradecharm.go:76: f.StringVar(&c.SwitchURL, "switch", "",
"charm URL to upgrade to")
"crossgrade to a different charm" ?

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode77
cmd/juju/upgradecharm.go:77: f.IntVar(&c.Revision, "revision", -1,
"revision number to upgrade to")
"explicit revision of current charm" ?

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode161
cmd/juju/upgradecharm.go:161: if considerBumpRevision {
On 2013/04/29 09:53:21, dimitern wrote:
> On 2013/04/28 10:41:25, fwereade wrote:
> > I think this can all be simpler.
> >
> > oldURL, _ := service.CharmURL()
> > var newURL *charm.URL
> > if c.SwitchURL != "" {
> > // A new charm URL was explicitly specified.
> > conf, err := conn.State.EnvironConfig()
> > if err != nil {
> > return err
> > }
> > ...

Read more...

Revision history for this message
Dimiter Naydenov (dimitern) wrote :
Download full text (5.4 KiB)

Please take a look.

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

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#oldcode95
cmd/juju/upgradecharm.go:95: return fmt.Errorf("already running latest
charm %q", curl)
On 2013/04/29 10:45:26, fwereade wrote:
> On 2013/04/29 09:53:21, dimitern wrote:
> > On 2013/04/28 10:41:25, fwereade wrote:
> > > Hmm. If the --force flag is set to something different to before,
 we might
> > > actually want to allow this case (and other error below)?.
 Pre-existing bug,
> > > doesn't have to be addressed in this CL, but if it isn't it should
 be
> > recorded.
> >
> > I added a TODO

> A bug would be great too please.

Done.

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

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode27
cmd/juju/upgradecharm.go:27: originally deployed.
On 2013/04/29 10:45:26, fwereade wrote:
> An explicit revision can be chosen with the --revision flag.

Done.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode50
cmd/juju/upgradecharm.go:50: The new charm may add new relations and
configuration settings.
On 2013/04/29 10:45:26, fwereade wrote:
> How about:

> The --switch flag allows you to replace the charm with an entirely
different
> one. The new charm's URL and revision are inferred as they would be
when running
> a deploy command.

> Please note that --switch is dangerous, because juju only has limited
> information with which to determine compatibility; the operation will
succeed,
> regardless of potential havoc, so long as the following conditions
hold:

> - The new charm must declare all relations that the service is
currently
> participating in.
> - All config settings shared by the old and new charms must have the
same types.

> --switch and --revision are mutually exclusive.

Done.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode56
cmd/juju/upgradecharm.go:56: specify revision number 5 of the wordpress
charm.
On 2013/04/29 10:45:26, fwereade wrote:
> Drop this para, --revision is best described at the top, I think; we
 just need
> to mention it won't work with switch.

I changed the wording to better describe both.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode76
cmd/juju/upgradecharm.go:76: f.StringVar(&c.SwitchURL, "switch", "",
"charm URL to upgrade to")
On 2013/04/29 10:45:26, fwereade wrote:
> "crossgrade to a different charm" ?

ewww.. ok :)

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode77
cmd/juju/upgradecharm.go:77: f.IntVar(&c.Revision, "revision", -1,
"revision number to upgrade to")
On 2013/04/29 10:45:26, fwereade wrote:
> "explicit revision of current charm" ?

Done.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode161
cmd/juju/upgradecharm.go:161: if considerBumpRevision {
On 2013/04/29 10:45:26, fwereade wrote:
> On 2013/04/29 09:53:21, dimitern wrote:
> > On 2013/04/28 10:41:25, fwereade wrote:
> > > I th...

Read more...

Revision history for this message
William Reade (fwereade) wrote :

nearly there, last round I think

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

https://codereview.appspot.com/8540050/diff/32001/cmd/juju/upgradecharm.go#oldcode100
cmd/juju/upgradecharm.go:100: } else if _, bumpRevision =
ch.(*charm.Dir); !bumpRevision {
...here -- in which case we should only set bumpRevision if
explicitRevision is false.

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

https://codereview.appspot.com/8540050/diff/32001/cmd/juju/upgradecharm.go#newcode54
cmd/juju/upgradecharm.go:54: The new charm may add new relations and
configuration settings.
I think we can drop this line.

https://codereview.appspot.com/8540050/diff/32001/cmd/juju/upgradecharm.go#newcode145
cmd/juju/upgradecharm.go:145: if *newURL == *oldURL && !explicitRevision
{
If newURL matches oldURL, we have to take this branch. We don't need to
worry about explicitRevision until...

https://codereview.appspot.com/8540050/diff/32001/cmd/juju/upgradecharm.go#newcode162
cmd/juju/upgradecharm.go:162: sch, err := conn.PutCharm(newURL, repo,
bumpRevision)
It's somewhat crazy that we can specify a URL with an additional param
that means "lol not really". But that's way out of scope.

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

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

really close. just one final piece of logic to get right, i think.

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

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode76
cmd/juju/upgradecharm.go:76: f.StringVar(&c.SwitchURL, "switch", "",
"charm URL to upgrade to")
On 2013/04/29 12:05:56, dimitern wrote:
> On 2013/04/29 10:45:26, fwereade wrote:
> > "crossgrade to a different charm" ?

> ewww.. ok :)

crossgrade??

how about just "change" ?

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

https://codereview.appspot.com/8540050/diff/32001/cmd/juju/upgradecharm.go#newcode51
cmd/juju/upgradecharm.go:51: - All config settings shared by the old and
new charms must have the
that's not quite right, is it? i believe the new charm must declare all
the settings declared by the old, whereas this seems to imply that only
the intersection of old and new matters.

but maybe we the error message will be suitably self-explanatory if this
happens.

https://codereview.appspot.com/8540050/diff/32001/cmd/juju/upgradecharm.go#newcode145
cmd/juju/upgradecharm.go:145: if *newURL == *oldURL && !explicitRevision
{
as discussed online, i'm not sure this is quite right yet.

[16:53:37] <rogpeppe> fwereade: but the case i'm thinking of is
something like this:
[16:54:14] <rogpeppe> fwereade: we have a local copy of a charm, deploy
it, push it to the charm store, switch to that, then make some changes
to the local copy and switch back to that
[16:54:48] <rogpeppe> fwereade: in that case, we want the version to be
bumped, but the urls are different

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

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

Please take a look.

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

Revision history for this message
William Reade (fwereade) wrote :

LGTM with suggestions

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

https://codereview.appspot.com/8540050/diff/25002/cmd/juju/upgradecharm.go#oldcode100
cmd/juju/upgradecharm.go:100: } else if _, bumpRevision =
ch.(*charm.Dir); !bumpRevision {
We could just not bother checking this, and always setting bumpRevision
in local repos -- I don't think many people are putting bundles in local
repos anyway, and the error message from PutCharm should be clear enough
to anyone who is.

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

https://codereview.appspot.com/8540050/diff/25002/cmd/juju/upgradecharm.go#newcode153
cmd/juju/upgradecharm.go:153: // case (and the other error below). LP
bug #1174287
Move this comment up to the top case.

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

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

*** Submitted:

cmd/juju: upgrade-charm --switch support

This implements a new argument to upgrade-charm:
--switch <charm-url>. The passed URL is inferred
to get the complete URL and used instead of the
service's newest charm url revision.
Also --revision is now supported, to give an
explicit revision to upgrade to, rather than the
latest.

There are a few related sanity checks that need
to be implemented as well (in follow-ups), so we
can better check charm compatibility before upgrading.

Fixes bugs #1040210 and #1050750.

R=TheMue, rog, fwereade
CC=
https://codereview.appspot.com/8540050

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'cmd/juju/upgradecharm.go'
2--- cmd/juju/upgradecharm.go 2013-04-25 10:46:51 +0000
3+++ cmd/juju/upgradecharm.go 2013-04-29 16:31:27 +0000
4@@ -17,24 +17,50 @@
5 ServiceName string
6 Force bool
7 RepoPath string // defaults to JUJU_REPOSITORY
8+ SwitchURL string
9+ Revision int // defaults to -1 (latest)
10 }
11
12 const upgradeCharmDoc = `
13-When no flags are set, the service's charm will be upgraded to the latest
14-revision available in the repository from which it was originally deployed.
15-
16-If the charm came from a local repository, its path will be assumed to be
17-$JUJU_REPOSITORY unless overridden by --repository. If there is no newer
18-revision of a local charm directory, the local directory's revision will be
19-automatically incremented to create a newer charm.
20-
21-The local repository behaviour is tuned specifically to the workflow of a charm
22-author working on a single client machine; use of local repositories from
23-multiple clients is not supported and may lead to confusing behaviour.
24-
25-Use of the --force flag is not generally recommended; units upgraded while in
26-an error state will not have upgrade-charm hooks executed, and may cause
27-unexpected behavior.
28+When no flags are set, the service's charm will be upgraded to the
29+latest revision available in the repository from which it was
30+originally deployed. An explicit revision can be chosen with the
31+--revision flag.
32+
33+If the charm came from a local repository, its path will be assumed to
34+be $JUJU_REPOSITORY unless overridden by --repository. If there is no
35+newer revision of a local charm directory, the local directory's
36+revision will be automatically incremented to create a newer charm.
37+
38+The local repository behaviour is tuned specifically to the workflow
39+of a charm author working on a single client machine; use of local
40+repositories from multiple clients is not supported and may lead to
41+confusing behaviour.
42+
43+The --switch flag allows you to replace the charm with an entirely
44+different one. The new charm's URL and revision are inferred as they
45+would be when running a deploy command.
46+
47+Please note that --switch is dangerous, because juju only has limited
48+information with which to determine compatibility; the operation will
49+succeed, regardless of potential havoc, so long as the following
50+conditions hold:
51+
52+- The new charm must declare all relations that the service is
53+currently participating in.
54+- All config settings shared by the old and new charms must have the
55+same types.
56+
57+The new charm may add new relations and configuration settings.
58+
59+--switch and --revision are mutually exclusive. To specify a given
60+revision number with --switch, give it in the charm URL, for instance
61+"cs:wordpress-5" would specify revision number 5 of the wordpress
62+charm.
63+
64+Use of the --force flag is not generally recommended; units upgraded
65+while in an error state will not have upgrade-charm hooks executed,
66+and may cause unexpected behavior.
67 `
68
69 func (c *UpgradeCharmCommand) Info() *cmd.Info {
70@@ -50,6 +76,8 @@
71 c.EnvCommandBase.SetFlags(f)
72 f.BoolVar(&c.Force, "force", false, "upgrade all units immediately, even if in error state")
73 f.StringVar(&c.RepoPath, "repository", os.Getenv("JUJU_REPOSITORY"), "local charm repository path")
74+ f.StringVar(&c.SwitchURL, "switch", "", "crossgrade to a different charm")
75+ f.IntVar(&c.Revision, "revision", -1, "explicit revision of current charm")
76 }
77
78 func (c *UpgradeCharmCommand) Init(args []string) error {
79@@ -64,7 +92,9 @@
80 default:
81 return cmd.CheckEmpty(args[1:])
82 }
83- // TODO(dimitern): add the other flags --switch and --revision.
84+ if c.SwitchURL != "" && c.Revision != -1 {
85+ return fmt.Errorf("--switch and --revision are mutually exclusive")
86+ }
87 return nil
88 }
89
90@@ -80,29 +110,58 @@
91 if err != nil {
92 return err
93 }
94- curl, _ := service.CharmURL()
95- repo, err := charm.InferRepository(curl, ctx.AbsPath(c.RepoPath))
96- if err != nil {
97- return err
98- }
99- rev, err := repo.Latest(curl)
100- if err != nil {
101- return err
102+ oldURL, _ := service.CharmURL()
103+ var newURL *charm.URL
104+ if c.SwitchURL != "" {
105+ // A new charm URL was explicitly specified.
106+ conf, err := conn.State.EnvironConfig()
107+ if err != nil {
108+ return err
109+ }
110+ newURL, err = charm.InferURL(c.SwitchURL, conf.DefaultSeries())
111+ if err != nil {
112+ return err
113+ }
114+ } else {
115+ // No new URL specified, but revision might have been.
116+ newURL = oldURL.WithRevision(c.Revision)
117+ }
118+ repo, err := charm.InferRepository(newURL, ctx.AbsPath(c.RepoPath))
119+ if err != nil {
120+ return err
121+ }
122+ // If no explicit revision was set with either SwitchURL
123+ // or Revision flags, discover the latest.
124+ explicitRevision := true
125+ if newURL.Revision == -1 {
126+ explicitRevision = false
127+ latest, err := repo.Latest(newURL)
128+ if err != nil {
129+ return err
130+ }
131+ newURL = newURL.WithRevision(latest)
132 }
133 bumpRevision := false
134- if curl.Revision == rev {
135+ if *newURL == *oldURL {
136+ if explicitRevision {
137+ return fmt.Errorf("already running specified charm %q", newURL)
138+ }
139+ // Only try bumping the revision when necessary (local dir charm).
140 if _, isLocal := repo.(*charm.LocalRepository); !isLocal {
141- return fmt.Errorf("already running latest charm %q", curl)
142+ // TODO(dimitern): If the --force flag is set to something
143+ // different to before, we might actually want to allow this
144+ // case (and the other error below). LP bug #1174287
145+ return fmt.Errorf("already running latest charm %q", newURL)
146 }
147 // This is a local repository.
148- if ch, err := repo.Get(curl); err != nil {
149+ if ch, err := repo.Get(newURL); err != nil {
150 return err
151 } else if _, bumpRevision = ch.(*charm.Dir); !bumpRevision {
152 // Only bump the revision when it's a directory.
153- return fmt.Errorf("already running latest charm %q", curl)
154+ return fmt.Errorf("already running latest charm %q", newURL)
155 }
156 }
157- sch, err := conn.PutCharm(curl.WithRevision(rev), repo, bumpRevision)
158+ sch, err := conn.PutCharm(newURL, repo, bumpRevision)
159 if err != nil {
160 return err
161 }
162
163=== modified file 'cmd/juju/upgradecharm_test.go'
164--- cmd/juju/upgradecharm_test.go 2013-03-27 15:42:27 +0000
165+++ cmd/juju/upgradecharm_test.go 2013-04-29 16:31:27 +0000
166@@ -54,10 +54,38 @@
167 testing.Charms.BundlePath(s.seriesPath, "riak")
168 err := runDeploy(c, "local:riak", "riak")
169 c.Assert(err, IsNil)
170+
171 err = runUpgradeCharm(c, "riak")
172 c.Assert(err, ErrorMatches, `already running latest charm "local:precise/riak-7"`)
173 }
174
175+func (s *UpgradeCharmErrorsSuite) deployService(c *C) {
176+ testing.Charms.ClonedDirPath(s.seriesPath, "riak")
177+ err := runDeploy(c, "local:riak", "riak")
178+ c.Assert(err, IsNil)
179+}
180+
181+func (s *UpgradeCharmErrorsSuite) TestInvalidSwitchURL(c *C) {
182+ s.deployService(c)
183+ err := runUpgradeCharm(c, "riak", "--switch=blah")
184+ c.Assert(err, ErrorMatches, "charm not found: cs:precise/blah")
185+ err = runUpgradeCharm(c, "riak", "--switch=cs:missing/one")
186+ c.Assert(err, ErrorMatches, "charm not found: cs:missing/one")
187+ // TODO(dimitern): add tests with incompatible charms
188+}
189+
190+func (s *UpgradeCharmErrorsSuite) TestSwitchAndRevisionFails(c *C) {
191+ s.deployService(c)
192+ err := runUpgradeCharm(c, "riak", "--switch=riak", "--revision=2")
193+ c.Assert(err, ErrorMatches, "--switch and --revision are mutually exclusive")
194+}
195+
196+func (s *UpgradeCharmErrorsSuite) TestInvalidRevision(c *C) {
197+ s.deployService(c)
198+ err := runUpgradeCharm(c, "riak", "--revision=blah")
199+ c.Assert(err, ErrorMatches, `invalid value "blah" for flag --revision: strconv.ParseInt: parsing "blah": invalid syntax`)
200+}
201+
202 type UpgradeCharmSuccessSuite struct {
203 repoSuite
204 path string
205@@ -79,7 +107,7 @@
206 c.Assert(forced, Equals, false)
207 }
208
209-func (s *UpgradeCharmSuccessSuite) assertUpgraded(c *C, revision int, forced bool) {
210+func (s *UpgradeCharmSuccessSuite) assertUpgraded(c *C, revision int, forced bool) *charm.URL {
211 err := s.riak.Refresh()
212 c.Assert(err, IsNil)
213 ch, force, err := s.riak.Charm()
214@@ -87,10 +115,11 @@
215 c.Assert(ch.Revision(), Equals, revision)
216 c.Assert(force, Equals, forced)
217 s.assertCharmUploaded(c, ch.URL())
218+ return ch.URL()
219 }
220
221-func (s *UpgradeCharmSuccessSuite) assertLocalRevision(c *C, revision int) {
222- dir, err := charm.ReadDir(s.path)
223+func (s *UpgradeCharmSuccessSuite) assertLocalRevision(c *C, revision int, path string) {
224+ dir, err := charm.ReadDir(path)
225 c.Assert(err, IsNil)
226 c.Assert(dir.Revision(), Equals, revision)
227 }
228@@ -99,7 +128,7 @@
229 err := runUpgradeCharm(c, "riak")
230 c.Assert(err, IsNil)
231 s.assertUpgraded(c, 8, false)
232- s.assertLocalRevision(c, 8)
233+ s.assertLocalRevision(c, 8, s.path)
234 }
235
236 func (s *UpgradeCharmSuccessSuite) TestDoesntBumpRevisionWhenNotNecessary(c *C) {
237@@ -111,7 +140,7 @@
238 err = runUpgradeCharm(c, "riak")
239 c.Assert(err, IsNil)
240 s.assertUpgraded(c, 42, false)
241- s.assertLocalRevision(c, 42)
242+ s.assertLocalRevision(c, 42, s.path)
243 }
244
245 func (s *UpgradeCharmSuccessSuite) TestUpgradesWithBundle(c *C) {
246@@ -124,17 +153,63 @@
247 bundlePath := path.Join(s.seriesPath, "riak.charm")
248 err = ioutil.WriteFile(bundlePath, buf.Bytes(), 0644)
249 c.Assert(err, IsNil)
250- c.Logf("%q %q", bundlePath, s.seriesPath)
251
252 err = runUpgradeCharm(c, "riak")
253 c.Assert(err, IsNil)
254 s.assertUpgraded(c, 42, false)
255- s.assertLocalRevision(c, 7)
256+ s.assertLocalRevision(c, 7, s.path)
257 }
258
259 func (s *UpgradeCharmSuccessSuite) TestForcedUpgrade(c *C) {
260 err := runUpgradeCharm(c, "riak", "--force")
261 c.Assert(err, IsNil)
262 s.assertUpgraded(c, 8, true)
263- s.assertLocalRevision(c, 8)
264+ s.assertLocalRevision(c, 8, s.path)
265+}
266+
267+var myriakMeta = []byte(`
268+name: myriak
269+summary: "K/V storage engine"
270+description: "Scalable K/V Store in Erlang with Clocks :-)"
271+provides:
272+ endpoint:
273+ interface: http
274+ admin:
275+ interface: http
276+peers:
277+ ring:
278+ interface: riak
279+`)
280+
281+func (s *UpgradeCharmSuccessSuite) TestSwitch(c *C) {
282+ myriakPath := testing.Charms.RenamedClonedDirPath(s.seriesPath, "riak", "myriak")
283+ err := ioutil.WriteFile(path.Join(myriakPath, "metadata.yaml"), myriakMeta, 0644)
284+ c.Assert(err, IsNil)
285+
286+ // Test with local repo and no explicit revsion.
287+ err = runUpgradeCharm(c, "riak", "--switch=local:myriak")
288+ c.Assert(err, IsNil)
289+ curl := s.assertUpgraded(c, 7, false)
290+ c.Assert(curl.String(), Equals, "local:precise/myriak-7")
291+ s.assertLocalRevision(c, 7, myriakPath)
292+
293+ // Try it again without revision - should be bumped.
294+ err = runUpgradeCharm(c, "riak", "--switch=local:myriak")
295+ c.Assert(err, IsNil)
296+ curl = s.assertUpgraded(c, 8, false)
297+ c.Assert(curl.String(), Equals, "local:precise/myriak-8")
298+ s.assertLocalRevision(c, 8, myriakPath)
299+
300+ // Now try the same with explicit revision - should fail.
301+ err = runUpgradeCharm(c, "riak", "--switch=local:myriak-8")
302+ c.Assert(err, ErrorMatches, `already running specified charm "local:precise/myriak-8"`)
303+
304+ // Change the revision to 42 and upgrade to it with explicit revision.
305+ err = ioutil.WriteFile(path.Join(myriakPath, "revision"), []byte("42"), 0644)
306+ c.Assert(err, IsNil)
307+ err = runUpgradeCharm(c, "riak", "--switch=local:myriak-42")
308+ c.Assert(err, IsNil)
309+ curl = s.assertUpgraded(c, 42, false)
310+ c.Assert(curl.String(), Equals, "local:precise/myriak-42")
311+ s.assertLocalRevision(c, 42, myriakPath)
312 }
313
314=== modified file 'testing/charm.go'
315--- testing/charm.go 2013-02-11 05:54:45 +0000
316+++ testing/charm.go 2013-04-29 16:31:27 +0000
317@@ -54,6 +54,15 @@
318 return clone(dst, r.DirPath(name))
319 }
320
321+// RenamedClonedDirPath returns the path to a new copy of the default
322+// charm directory named name, but renames it to newName.
323+func (r *Repo) RenamedClonedDirPath(dst, name, newName string) string {
324+ newDst := clone(dst, r.DirPath(name))
325+ renamedDst := filepath.Join(filepath.Dir(newDst), newName)
326+ check(os.Rename(newDst, renamedDst))
327+ return renamedDst
328+}
329+
330 // ClonedDir returns an actual charm.Dir based on a new copy of the charm directory
331 // named name, in the directory dst.
332 func (r *Repo) ClonedDir(dst, name string) *charm.Dir {

Subscribers

People subscribed via source and target branches