upgrade-charm now allows upgrading local charms

This fixes lp:1609463.

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.

api/application/client.go

@@ -61,29 +61,40 @@ func (c *Client) ModelUUID() string {
 
 // DeployArgs holds the arguments to be sent to Client.ServiceDeploy.
 type DeployArgs struct {
+
 	// CharmID identifies the charm to deploy.
 	CharmID charmstore.CharmID
+
 	// ApplicationName is the name to give the application.
 	ApplicationName string
+
 	// Series to be used for the machine.
 	Series string
+
 	// NumUnits is the number of units to deploy.
 	NumUnits int
+
 	// ConfigYAML is a string that overrides the default config.yml.
 	ConfigYAML string
-	// Cons contains constraints on where units of this application may be
-	// placed.
+
+	// Cons contains constraints on where units of this application
+	// may be placed.
 	Cons constraints.Value
+
 	// Placement directives on where the machines for the unit must be
 	// created.
 	Placement []*instance.Placement
+
 	// Storage contains Constraints specifying how storage should be
 	// handled.
 	Storage map[string]storage.Constraints
+
 	// EndpointBindings
 	EndpointBindings map[string]string
-	// Collection of resource names for the application, with the value being the
-	// unique ID of a pre-uploaded resources in storage.
+
+	// Collection of resource names for the application, with the
+	// value being the unique ID of a pre-uploaded resources in
+	// storage.
 	Resources map[string]string
 }
 
@@ -110,9 +121,9 @@ func (c *Client) Deploy(args DeployArgs) error {
 	var err error
 	err = c.facade.FacadeCall("Deploy", deployArgs, &results)
 	if err != nil {
-		return err
+		return errors.Trace(err)
 	}
-	return results.OneError()
+	return errors.Trace(results.OneError())
 }
 
 // GetCharmURL returns the charm URL the given service is
@@ -122,10 +133,10 @@ func (c *Client) GetCharmURL(serviceName string) (*charm.URL, error) {
 	args := params.ApplicationGet{ApplicationName: serviceName}
 	err := c.facade.FacadeCall("GetCharmURL", args, result)
 	if err != nil {
-		return nil, err
+		return nil, errors.Trace(err)
 	}
 	if result.Error != nil {
-		return nil, result.Error
+		return nil, errors.Trace(result.Error)
 	}
 	return charm.ParseURL(result.Result)
 }

cmd/juju/application/deploy.go

@@ -675,20 +675,6 @@ func (c *DeployCommand) Run(ctx *cmd.Context) error {
 	return block.ProcessBlockedError(deploy(ctx, apiRoot), block.BlockChange)
 }
 
-func (c *DeployCommand) newResolver(apiRoot DeployAPI) (*config.Config, *csclient.Client, error) {
-	bakeryClient, err := c.BakeryClient()
-	if err != nil {
-		return nil, nil, errors.Trace(err)
-	}
-	csClient := newCharmStoreClient(bakeryClient).WithChannel(c.Channel)
-	conf, err := getModelConfig(apiRoot)
-	if err != nil {
-		return nil, nil, errors.Trace(err)
-	}
-
-	return conf, csClient, nil
-}
-
 func findDeployerFIFO(maybeDeployers ...func() (deployFn, error)) (deployFn, error) {
 	for _, d := range maybeDeployers {
 		if deploy, err := d(); err != nil {

cmd/juju/application/upgradecharm.go

@@ -6,7 +6,6 @@ package application
 import (
 	"fmt"
 	"os"
-	"path/filepath"
 	"strings"
 
 	"github.com/juju/cmd"
@@ -269,7 +268,7 @@ func (c *upgradeCharmCommand) Run(ctx *cmd.Context) error {
 	charmUpgradeClient := c.NewCharmUpgradeClient(apiRoot)
 	oldURL, err := charmUpgradeClient.GetCharmURL(c.ApplicationName)
 	if err != nil {
-		return err
+		return errors.Trace(err)
 	}
 
 	newRef := c.SwitchURL
@@ -300,9 +299,15 @@ func (c *upgradeCharmCommand) Run(ctx *cmd.Context) error {
 	charmRepo := c.getCharmStore(bakeryClient, modelConfig)
 	chID, csMac, err := c.addCharm(charmAdder, charmRepo, modelConfig, oldURL, newRef)
 	if err != nil {
-		if err1, ok := errors.Cause(err).(*termsRequiredError); ok {
-			terms := strings.Join(err1.Terms, " ")
-			return errors.Errorf(`Declined: please agree to the following terms %s. Try: "juju agree %s"`, terms, terms)
+		if termsErr, ok := errors.Cause(err).(*termsRequiredError); ok {
+			terms := strings.Join(termsErr.Terms, " ")
+			return errors.Wrap(
+				termsErr,
+				errors.Errorf(
+					`Declined: please agree to the following terms %s. Try: "juju agree %[1]s"`,
+					terms,
+				),
+			)
 		}
 		return block.ProcessBlockedError(err, block.BlockChange)
 	}
@@ -499,7 +504,7 @@ func (c *upgradeCharmCommand) addCharm(
 	// Charm may have been supplied via a path reference.
 	ch, newURL, err := charmrepo.NewCharmAtPathForceSeries(charmRef, oldURL.Series, c.ForceSeries)
 	if err == nil {
-		_, newName := filepath.Split(charmRef)
+		newName := ch.Meta().Name
 		if newName != oldURL.Name {
 			return id, nil, errors.Errorf("cannot upgrade %q to %q", oldURL.Name, newName)
 		}

cmd/juju/application/upgradecharm_resources_test.go

@@ -55,8 +55,9 @@ func (s *UpgradeCharmResourceSuite) SetUpTest(c *gc.C) {
 	c.Assert(forced, jc.IsFalse)
 }
 
-var riakResourceMeta = []byte(`
-name: riakresource
+func (s *UpgradeCharmResourceSuite) TestUpgradeWithResources(c *gc.C) {
+	const riakResourceMeta = `
+name: riak
 summary: "K/V storage engine"
 description: "Scalable K/V Store in Erlang with Clocks :-)"
 provides:
@@ -72,11 +73,10 @@ resources:
     type: file
     filename: foo.lib
     description: some comment
-`)
+`
 
-func (s *UpgradeCharmResourceSuite) TestUpgradeWithResources(c *gc.C) {
 	myriakPath := testcharms.Repo.ClonedDir(c.MkDir(), "riak")
-	err := ioutil.WriteFile(path.Join(myriakPath.Path, "metadata.yaml"), riakResourceMeta, 0644)
+	err := ioutil.WriteFile(path.Join(myriakPath.Path, "metadata.yaml"), []byte(riakResourceMeta), 0644)
 	c.Assert(err, jc.ErrorIsNil)
 
 	data := []byte("some-data")

cmd/juju/application/upgradecharm_test.go

@@ -7,14 +7,14 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net/http/httptest"
+	"os"
 	"path"
 	"path/filepath"
 
 	"github.com/juju/cmd"
 	"github.com/juju/errors"
 	"github.com/juju/testing"
 	jc "github.com/juju/testing/checkers"
-	"github.com/juju/utils"
 	"github.com/juju/version"
 	gc "gopkg.in/check.v1"
 	"gopkg.in/juju/charm.v6-unstable"
@@ -26,6 +26,8 @@ import (
 	"gopkg.in/macaroon-bakery.v1/httpbakery"
 	macaroon "gopkg.in/macaroon.v1"
 
+	"strings"
+
 	"github.com/juju/juju/api"
 	"github.com/juju/juju/api/application"
 	"github.com/juju/juju/api/base"
@@ -409,25 +411,41 @@ func (s *UpgradeCharmSuccessStateSuite) TestForcedSeriesUpgrade(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(ch.Revision(), gc.Equals, 1)
 
-	// Copy files from a charm supporting a different set of series
-	// so we can try an upgrade requiring --force-series.
-	for _, f := range []string{"metadata.yaml", "revision"} {
-		err = utils.CopyFile(
-			filepath.Join(path, f),
-			filepath.Join(testcharms.Repo.CharmDirPath("multi-series2"), f))
-		c.Assert(err, jc.ErrorIsNil)
+	// Overwrite the metadata.yaml to change the supported series.
+	metadataPath := filepath.Join(path, "metadata.yaml")
+	file, err := os.OpenFile(metadataPath, os.O_TRUNC|os.O_RDWR, 0666)
+	if err != nil {
+		c.Fatal(errors.Annotate(err, "cannot open metadata.yaml for overwriting"))
+	}
+	defer file.Close()
+
+	metadata := strings.Join(
+		[]string{
+			`name: multi-series`,
+			`summary: "That's a dummy charm with multi-series."`,
+			`description: |`,
+			`    This is a longer description which`,
+			`    potentially contains multiple lines.`,
+			`series:`,
+			`    - trusty`,
+			`    - wily`,
+		},
+		"\n",
+	)
+	if _, err := file.WriteString(metadata); err != nil {
+		c.Fatal(errors.Annotate(err, "cannot write to metadata.yaml"))
 	}
+
 	err = runUpgradeCharm(c, "multi-series", "--path", path, "--force-series")
 	c.Assert(err, jc.ErrorIsNil)
 
 	err = application.Refresh()
 	c.Assert(err, jc.ErrorIsNil)
+
 	ch, force, err := application.Charm()
 	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(ch.Revision(), gc.Equals, 8)
-	c.Assert(force, gc.Equals, false)
-	s.AssertCharmUploaded(c, ch.URL())
-	c.Assert(ch.URL().String(), gc.Equals, "local:precise/multi-series2-8")
+	c.Check(ch.Revision(), gc.Equals, 2)
+	c.Check(force, gc.Equals, false)
 }
 
 func (s *UpgradeCharmSuccessStateSuite) TestInitWithResources(c *gc.C) {
@@ -500,7 +518,19 @@ func (s *UpgradeCharmSuccessStateSuite) TestCharmPathNoRevUpgrade(c *gc.C) {
 
 func (s *UpgradeCharmSuccessStateSuite) TestCharmPathDifferentNameFails(c *gc.C) {
 	myriakPath := testcharms.Repo.RenamedClonedDirPath(s.CharmsPath, "riak", "myriak")
-	err := runUpgradeCharm(c, "riak", "--path", myriakPath)
+	metadataPath := filepath.Join(myriakPath, "metadata.yaml")
+	file, err := os.OpenFile(metadataPath, os.O_TRUNC|os.O_RDWR, 0666)
+	if err != nil {
+		c.Fatal(errors.Annotate(err, "cannot open metadata.yaml"))
+	}
+	defer file.Close()
+
+	// Overwrite the metadata.yaml to contain a new name.
+	newMetadata := strings.Join([]string{`name: myriak`, `summary: ""`, `description: ""`}, "\n")
+	if _, err := file.WriteString(newMetadata); err != nil {
+		c.Fatal("cannot write to metadata.yaml")
+	}
+	err = runUpgradeCharm(c, "riak", "--path", myriakPath)
 	c.Assert(err, gc.ErrorMatches, `cannot upgrade "riak" to "myriak"`)
 }