Addressed review comments

jangocheng · Jul 25, 2018 · f788577 · f788577
1 parent ef92c79
commit f788577
Show file tree

Hide file tree

Showing 5 changed files with 56 additions and 79 deletions.
diff --git a/api/bundle/client.go b/api/bundle/client.go
@@ -38,7 +38,8 @@ func (c *Client) ExportBundle() (string, error) {
 	}
 
 	if err := c.facade.FacadeCall("ExportBundle", nil, &result); err != nil {
-		return "", errors.Annotate(result.Error, "export failed")
+		errors.Annotate(result.Error, "export failed")
+		return "", errors.Trace(err)
 	}
 
 	return result.Result, nil

diff --git a/api/bundle/client_test.go b/api/bundle/client_test.go
@@ -4,11 +4,13 @@
 package bundle_test
 
 import (
+	"github.com/juju/errors"
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 
 	basetesting "github.com/juju/juju/api/base/testing"
 	"github.com/juju/juju/api/bundle"
+	"github.com/juju/juju/apiserver/common"
 	"github.com/juju/juju/apiserver/params"
 	coretesting "github.com/juju/juju/testing"
 )
@@ -55,11 +57,6 @@ func (s *bundleMockSuite) TestExportBundlev2(c *gc.C) {
 			args,
 			response interface{},
 		) error {
-			c.Check(objType, gc.Equals, "Bundle")
-			c.Check(id, gc.Equals, "")
-			c.Check(request, gc.Equals, "ExportBundle")
-			c.Assert(args, gc.Equals, nil)
-			c.Assert(response, gc.FitsTypeOf, &params.StringResult{})
 			result := response.(*params.StringResult)
 			result.Result = "applications:\n  " +
 				"ubuntu:\n    " +
@@ -100,18 +97,14 @@ func (s *bundleMockSuite) TestExportBundleErrorv2(c *gc.C) {
 			args,
 			response interface{},
 		) error {
-			c.Check(objType, gc.Equals, "Bundle")
-			c.Check(id, gc.Equals, "")
-			c.Check(request, gc.Equals, "ExportBundle")
-			c.Assert(args, gc.Equals, nil)
-			c.Assert(response, gc.FitsTypeOf, &params.StringResult{})
 			result := response.(*params.StringResult)
 			result.Result = ""
+			*(response.(*params.StringResult)) = params.StringResult{Error: common.ServerError(errors.New("export failed"))}
 			return result.Error
 		}, 2,
 	)
 	result, err := client.ExportBundle()
 	c.Assert(err, gc.NotNil)
 	c.Assert(result, jc.DeepEquals, "")
-	c.Check(err.Error(), jc.Contains, "export failed")
+	c.Check(err.Error(), gc.Matches, "export failed")
 }
diff --git a/cmd/juju/model/export_test.go b/cmd/juju/model/export_test.go
@@ -67,7 +67,7 @@ func NewDumpDBCommandForTest(api DumpDBAPI, store jujuclient.ClientStore) cmd.Co
 	return modelcmd.Wrap(cmd)
 }
 
-// NewDumpCommandForTest returns a DumpCommand with the api provided as specified.
+// NewExportBundleCommandForTest returns a ExportBundleCommand with the api provided as specified.
 func NewExportBundleCommandForTest(api ExportBundleAPI, store jujuclient.ClientStore) cmd.Command {
 	cmd := &exportBundleCommand{newAPIFunc: func() (ExportBundleAPI, error) {
 		return api, nil

diff --git a/cmd/juju/model/exportbundle.go b/cmd/juju/model/exportbundle.go
@@ -28,25 +28,26 @@ type exportBundleCommand struct {
 	modelcmd.ModelCommandBase
 	out        cmd.Output
 	newAPIFunc func() (ExportBundleAPI, error)
-	// name of the charm bundle file.
-	Filename string
+	Filename   string
 }
 
 const exportBundleHelpDoc = `
-Exports the current model configuration into a YAML file.
+Exports the current model configuration as bundle.
+
+If --filename is optional. The charm bundle is written in the filename provided as value.
+If --filename is not used, the bundle is displayed in stdout.
 
 Examples:
 
     juju export-bundle
-
-If --filename is not used, the bundle is displayed in stdout.
+	juju export-bundle --filename mymodel
 `
 
 // Info implements Command.
 func (c *exportBundleCommand) Info() *cmd.Info {
 	return &cmd.Info{
 		Name:    "export-bundle",
-		Purpose: "Exports the current model configuration in a charm bundle.",
+		Purpose: "Exports the current model configuration as bundle.",
 		Doc:     exportBundleHelpDoc,
 	}
 }
@@ -55,18 +56,17 @@ func (c *exportBundleCommand) Info() *cmd.Info {
 func (c *exportBundleCommand) SetFlags(f *gnuflag.FlagSet) {
 	c.ModelCommandBase.SetFlags(f)
 	c.out.AddFlags(f, "yaml", output.DefaultFormatters)
-	f.StringVar(&c.Filename, "filename", "", "Export Model")
+	f.StringVar(&c.Filename, "filename", "", "bundle file")
 }
 
 // Init implements Command.
 func (c *exportBundleCommand) Init(args []string) error {
 	return cmd.CheckEmpty(args)
 }
 
-// ExportBundleAPI specifies the used function calls of the ModelManager.
+// ExportBundleAPI specifies the used function calls of the BundleFacade.
 type ExportBundleAPI interface {
 	Close() error
-	BestAPIVersion() int
 	ExportBundle() (string, error)
 }
 
@@ -102,13 +102,11 @@ func (c *exportBundleCommand) Run(ctx *cmd.Context) error {
 	}
 	defer file.Close()
 
-	// Write out the result.
 	_, err = file.WriteString(result)
 	if err != nil {
 		return errors.Annotate(err, "while copying in local file")
 	}
 
-	// Print the local filename.
 	fmt.Fprintln(ctx.Stdout, "Bundle successfully exported to", filename)
 
 	return nil

diff --git a/cmd/juju/model/exportbundle_test.go b/cmd/juju/model/exportbundle_test.go
@@ -4,8 +4,9 @@
 package model_test
 
 import (
+	"io/ioutil"
+
 	"github.com/juju/cmd/cmdtesting"
-	"github.com/juju/errors"
 	jujutesting "github.com/juju/testing"
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
@@ -25,21 +26,6 @@ type ExportBundleCommandSuite struct {
 
 var _ = gc.Suite(&ExportBundleCommandSuite{})
 
-func (f *fakeExportBundleClient) Close() error { return nil }
-
-func (f *fakeExportBundleClient) ExportBundle() (string, error) {
-	f.MethodCall(f, "ExportBundle")
-	if err := f.NextErr(); err != nil {
-		return "", err
-	}
-
-	return f.result, f.NextErr()
-}
-
-func (f *fakeExportBundleClient) BestAPIVersion() int {
-	return f.bestAPIVersion
-}
-
 func (s *ExportBundleCommandSuite) SetUpTest(c *gc.C) {
 	s.FakeJujuXDGDataHomeSuite.SetUpTest(c)
 	s.stub = &jujutesting.Stub{}
@@ -60,40 +46,6 @@ func (s *ExportBundleCommandSuite) SetUpTest(c *gc.C) {
 	s.store.Models["testing"].CurrentModel = "admin/mymodel"
 }
 
-func (s *ExportBundleCommandSuite) TestExportBundleFailOnv1(c *gc.C) {
-	s.fake.result = ""
-	s.fake.Stub.SetErrors(errors.New("command not supported on v1"))
-	s.fake.bestAPIVersion = 1
-
-	ctx, err := cmdtesting.RunCommand(c, model.NewExportBundleCommandForTest(s.fake, s.store))
-	c.Assert(err, gc.NotNil)
-
-	s.fake.CheckCalls(c, []jujutesting.StubCall{
-		{"ExportBundle", nil},
-	})
-
-	out := cmdtesting.Stdout(ctx)
-	c.Assert(out, gc.Equals, "")
-	c.Assert(err, gc.ErrorMatches, "command not supported on v1")
-}
-
-func (s *ExportBundleCommandSuite) TestExportBundleFailEmptyResult(c *gc.C) {
-	s.fake.result = ""
-	s.fake.Stub.SetErrors(errors.New("export failed: nothing to export as there are no applications"))
-	s.fake.bestAPIVersion = 2
-
-	ctx, err := cmdtesting.RunCommand(c, model.NewExportBundleCommandForTest(s.fake, s.store))
-	c.Assert(err, gc.NotNil)
-
-	s.fake.CheckCalls(c, []jujutesting.StubCall{
-		{"ExportBundle", nil},
-	})
-
-	out := cmdtesting.Stdout(ctx)
-	c.Assert(out, gc.Equals, "")
-	c.Assert(err, gc.ErrorMatches, "export failed: nothing to export as there are no applications")
-}
-
 func (s *ExportBundleCommandSuite) TestExportBundleSuccessNoFilename(c *gc.C) {
 	s.fake.result = "applications:\n" +
 		"  mysql:\n" +
@@ -114,7 +66,6 @@ func (s *ExportBundleCommandSuite) TestExportBundleSuccessNoFilename(c *gc.C) {
 		"relations:\n" +
 		"- - wordpress:db\n" +
 		"  - mysql:mysql\n"
-	s.fake.bestAPIVersion = 2
 
 	ctx, err := cmdtesting.RunCommand(c, model.NewExportBundleCommandForTest(s.fake, s.store))
 	c.Assert(err, jc.ErrorIsNil)
@@ -146,8 +97,18 @@ func (s *ExportBundleCommandSuite) TestExportBundleSuccessNoFilename(c *gc.C) {
 }
 
 func (s *ExportBundleCommandSuite) TestExportBundleSuccessFilename(c *gc.C) {
-	s.fake.bestAPIVersion = 2
-
+	s.fake.result = "applications:\n" +
+		"  magic:\n" +
+		"    charm: cs:zesty/magic\n" +
+		"    series: zesty\n" +
+		"    expose: true\n" +
+		"    options:\n" +
+		"      key: value\n" +
+		"    bindings:\n" +
+		"      rel-name: some-space\n" +
+		"series: xenial\n" +
+		"relations:\n" +
+		"- []\n"
 	ctx, err := cmdtesting.RunCommand(c, model.NewExportBundleCommandForTest(s.fake, s.store), "--filename", "mymodel")
 	c.Assert(err, jc.ErrorIsNil)
 	s.fake.CheckCalls(c, []jujutesting.StubCall{
@@ -156,10 +117,34 @@ func (s *ExportBundleCommandSuite) TestExportBundleSuccessFilename(c *gc.C) {
 
 	out := cmdtesting.Stdout(ctx)
 	c.Assert(out, gc.Equals, "Bundle successfully exported to mymodel.yaml\n")
+	output, err := ioutil.ReadFile("mymodel.yaml")
+	c.Check(err, jc.ErrorIsNil)
+	c.Assert(string(output), gc.Equals, "applications:\n"+
+		"  magic:\n"+
+		"    charm: cs:zesty/magic\n"+
+		"    series: zesty\n"+
+		"    expose: true\n"+
+		"    options:\n"+
+		"      key: value\n"+
+		"    bindings:\n"+
+		"      rel-name: some-space\n"+
+		"series: xenial\n"+
+		"relations:\n"+
+		"- []\n")
+}
+
+func (f *fakeExportBundleClient) Close() error { return nil }
+
+func (f *fakeExportBundleClient) ExportBundle() (string, error) {
+	f.MethodCall(f, "ExportBundle")
+	if err := f.NextErr(); err != nil {
+		return "", err
+	}
+
+	return f.result, f.NextErr()
 }
 
 type fakeExportBundleClient struct {
 	*jujutesting.Stub
-	bestAPIVersion int
-	result         string
+	result string
 }