Add DownloadOneFromChannelByName, improve config validation.

Fail config functions if id or name is an empty string.
sdg0919 · Oct 26, 2021 · 4a488d2 · 4a488d2
1 parent b758d11
commit 4a488d2
Show file tree

Hide file tree

Showing 2 changed files with 184 additions and 39 deletions.
diff --git a/charmhub/refresh.go b/charmhub/refresh.go
@@ -21,6 +21,7 @@ import (
 	"github.com/juju/juju/charmhub/path"
 	"github.com/juju/juju/charmhub/transport"
 	charmmetrics "github.com/juju/juju/core/charm/metrics"
+	corelogger "github.com/juju/juju/core/logger"
 	coreseries "github.com/juju/juju/core/series"
 )
 
@@ -156,7 +157,7 @@ func (c *RefreshClient) refresh(ctx context.Context, ensure func(responses []tra
 		return nil, errors.Trace(err)
 	}
 	if restResp.StatusCode == http.StatusNotFound {
-		return nil, errors.NotFoundf("refresh")
+		return nil, logAndReturnError(errors.NotFoundf("refresh"))
 	}
 	if err := handleBasicAPIErrors(resp.ErrorList, c.logger); err != nil {
 		return nil, errors.Trace(err)
@@ -168,18 +169,21 @@ func (c *RefreshClient) refresh(ctx context.Context, ensure func(responses []tra
 
 // RefreshOne creates a request config for requesting only one charm.
 func RefreshOne(key, id string, revision int, channel string, base RefreshBase) (RefreshConfig, error) {
+	if id == "" {
+		return nil, logAndReturnError(errors.NotValidf("empty id"))
+	}
 	if key == "" {
 		// This is for compatibility reasons.  With older clients, the
 		// key created in GetCharmURLOrigin will be lost to and from
 		// the client.  Since a key is required, ensure we have one.
 		uuid, err := utils.NewUUID()
 		if err != nil {
-			return nil, errors.Trace(err)
+			return nil, logAndReturnError(err)
 		}
 		key = uuid.String()
 	}
 	if err := validateBase(base); err != nil {
-		return nil, errors.Trace(err)
+		return nil, logAndReturnError(err)
 	}
 	return refreshOne{
 		instanceKey: key,
@@ -202,9 +206,12 @@ func CreateInstanceKey(app names.ApplicationTag, model names.ModelTag) string {
 // InstallOneFromRevision creates a request config using the revision and not
 // the channel for requesting only one charm.
 func InstallOneFromRevision(name string, revision int) (RefreshConfig, error) {
+	if name == "" {
+		return nil, logAndReturnError(errors.NotValidf("empty name"))
+	}
 	uuid, err := utils.NewUUID()
 	if err != nil {
-		return nil, errors.Trace(err)
+		return nil, logAndReturnError(err)
 	}
 	return executeOneByRevision{
 		action:      InstallAction,
@@ -252,12 +259,15 @@ func AddConfigMetrics(config RefreshConfig, metrics map[charmmetrics.MetricKey]s
 // InstallOneFromChannel creates a request config using the channel and not the
 // revision for requesting only one charm.
 func InstallOneFromChannel(name string, channel string, base RefreshBase) (RefreshConfig, error) {
+	if name == "" {
+		return nil, logAndReturnError(errors.NotValidf("empty name"))
+	}
 	if err := validateBase(base); err != nil {
-		return nil, errors.Trace(err)
+		return nil, logAndReturnError(err)
 	}
 	uuid, err := utils.NewUUID()
 	if err != nil {
-		return nil, errors.Trace(err)
+		return nil, logAndReturnError(err)
 	}
 	return executeOne{
 		action:      InstallAction,
@@ -268,45 +278,50 @@ func InstallOneFromChannel(name string, channel string, base RefreshBase) (Refre
 	}, nil
 }
 
-// DownloadOne creates a request config for requesting only one charm.
-func DownloadOne(id string, revision int, channel string, base RefreshBase) (RefreshConfig, error) {
-	if err := validateBase(base); err != nil {
-		return nil, errors.Trace(err)
+// DownloadOneFromRevision creates a request config using the revision and not
+// the channel for requesting only one charm.
+func DownloadOneFromRevision(id string, revision int) (RefreshConfig, error) {
+	if id == "" {
+		return nil, logAndReturnError(errors.NotValidf("empty id"))
 	}
 	uuid, err := utils.NewUUID()
 	if err != nil {
-		return nil, errors.Trace(err)
+		return nil, logAndReturnError(err)
 	}
-	return executeOne{
+	return executeOneByRevision{
 		action:      DownloadAction,
 		instanceKey: uuid.String(),
 		ID:          id,
 		Revision:    &revision,
-		Channel:     &channel,
-		Base:        base,
 	}, nil
 }
 
-// DownloadOneFromRevision creates a request config using the revision and not
+// DownloadOneFromRevisionByName creates a request config using the revision and not
 // the channel for requesting only one charm.
-func DownloadOneFromRevision(id string, revision int) (RefreshConfig, error) {
+func DownloadOneFromRevisionByName(name string, revision int) (RefreshConfig, error) {
+	if name == "" {
+		return nil, logAndReturnError(errors.NotValidf("empty name"))
+	}
 	uuid, err := utils.NewUUID()
 	if err != nil {
-		return nil, errors.Trace(err)
+		return nil, logAndReturnError(err)
 	}
 	return executeOneByRevision{
 		action:      DownloadAction,
 		instanceKey: uuid.String(),
-		ID:          id,
+		Name:        name,
 		Revision:    &revision,
 	}, nil
 }
 
 // DownloadOneFromChannel creates a request config using the channel and not the
 // revision for requesting only one charm.
 func DownloadOneFromChannel(id string, channel string, base RefreshBase) (RefreshConfig, error) {
+	if id == "" {
+		return nil, logAndReturnError(errors.NotValidf("empty id"))
+	}
 	if err := validateBase(base); err != nil {
-		return nil, errors.Trace(err)
+		return nil, logAndReturnError(err)
 	}
 	uuid, err := utils.NewUUID()
 	if err != nil {
@@ -321,11 +336,33 @@ func DownloadOneFromChannel(id string, channel string, base RefreshBase) (Refres
 	}, nil
 }
 
+// DownloadOneFromChannelByName creates a request config using the channel and not the
+// revision for requesting only one charm.
+func DownloadOneFromChannelByName(name string, channel string, base RefreshBase) (RefreshConfig, error) {
+	if name == "" {
+		return nil, logAndReturnError(errors.NotValidf("empty name"))
+	}
+	if err := validateBase(base); err != nil {
+		return nil, logAndReturnError(err)
+	}
+	uuid, err := utils.NewUUID()
+	if err != nil {
+		return nil, logAndReturnError(err)
+	}
+	return executeOne{
+		action:      DownloadAction,
+		instanceKey: uuid.String(),
+		Name:        name,
+		Channel:     &channel,
+		Base:        base,
+	}, nil
+}
+
 // constructRefreshBase creates a refresh request base that allows for
 // partial base queries.
 func constructRefreshBase(base RefreshBase) (transport.Base, error) {
 	if base.Architecture == "" {
-		return transport.Base{}, errors.NotValidf("refresh arch")
+		return transport.Base{}, logAndReturnError(errors.NotValidf("refresh arch"))
 	}
 
 	name := base.Name
@@ -381,9 +418,7 @@ func validateBase(rp RefreshBase) error {
 		err := errors.Trace(errors.NotValidf(strings.Join(msg, ", ")))
 		// Log the error here, trace on this side gets lost when the error
 		// goes thru to the client.
-		logger := loggo.GetLogger("juju.charmhub.validatebase")
-		logger.Errorf(fmt.Sprintf("%s", err))
-		return err
+		return logAndReturnError(err)
 	}
 	return nil
 }
@@ -401,3 +436,11 @@ func ExtractConfigInstanceKey(cfg RefreshConfig) string {
 	}
 	return ""
 }
+
+var logger = loggo.GetLoggerWithLabels("juju.charmhub", corelogger.CHARMHUB)
+
+func logAndReturnError(err error) error {
+	err = errors.Trace(err)
+	logger.Errorf(err.Error())
+	return err
+}
diff --git a/charmhub/refresh_test.go b/charmhub/refresh_test.go
@@ -461,6 +461,15 @@ func (s *RefreshConfigSuite) TestRefreshOneWithMetricsBuild(c *gc.C) {
 	})
 }
 
+func (s *RefreshConfigSuite) TestRefreshOneFail(c *gc.C) {
+	_, err := RefreshOne("", 1, "latest/stable", RefreshBase{
+		Name:         "ubuntu",
+		Channel:      "20.04",
+		Architecture: arch.DefaultArchitecture,
+	})
+	c.Assert(err, jc.Satisfies, errors.IsNotValid)
+}
+
 func (s *RefreshConfigSuite) TestRefreshOneEnsure(c *gc.C) {
 	config, err := RefreshOne("instance-key", "foo", 1, "latest/stable", RefreshBase{
 		Name:         "ubuntu",
@@ -475,7 +484,7 @@ func (s *RefreshConfigSuite) TestRefreshOneEnsure(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 }
 
-func (s *RefreshConfigSuite) TestInstallOneBuildRevision(c *gc.C) {
+func (s *RefreshConfigSuite) TestInstallOneFromRevisionBuild(c *gc.C) {
 	revision := 1
 
 	name := "foo"
@@ -498,6 +507,11 @@ func (s *RefreshConfigSuite) TestInstallOneBuildRevision(c *gc.C) {
 	})
 }
 
+func (s *RefreshConfigSuite) TestInstallOneFromRevisionFail(c *gc.C) {
+	_, err := InstallOneFromRevision("", 1)
+	c.Assert(err, jc.Satisfies, errors.IsNotValid)
+}
+
 func (s *RefreshConfigSuite) TestInstallOneBuildRevisionResources(c *gc.C) {
 	// Tests InstallOne by revision with specific resources.
 	revision := 1
@@ -569,6 +583,15 @@ func (s *RefreshConfigSuite) TestInstallOneBuildChannel(c *gc.C) {
 	})
 }
 
+func (s *RefreshConfigSuite) TestInstallOneChannelFail(c *gc.C) {
+	_, err := InstallOneFromChannel("", "stable", RefreshBase{
+		Name:         "ubuntu",
+		Channel:      "20.04",
+		Architecture: arch.DefaultArchitecture,
+	})
+	c.Assert(err, jc.Satisfies, errors.IsNotValid)
+}
+
 func (s *RefreshConfigSuite) TestInstallOneWithPartialPlatform(c *gc.C) {
 	channel := "latest/stable"
 
@@ -611,7 +634,7 @@ func (s *RefreshConfigSuite) TestInstallOneWithMissingArch(c *gc.C) {
 	c.Assert(errors.IsNotValid(err), jc.IsTrue)
 }
 
-func (s *RefreshConfigSuite) TestInstallOneEnsure(c *gc.C) {
+func (s *RefreshConfigSuite) TestInstallOneFromChannelEnsure(c *gc.C) {
 	config, err := InstallOneFromChannel("foo", "latest/stable", RefreshBase{
 		Name:         "ubuntu",
 		Channel:      "20.04",
@@ -627,36 +650,67 @@ func (s *RefreshConfigSuite) TestInstallOneEnsure(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 }
 
-func (s *RefreshConfigSuite) TestInstallOneFromChannelEnsure(c *gc.C) {
-	config, err := InstallOneFromChannel("foo", "latest/stable", RefreshBase{
+func (s *RefreshConfigSuite) TestInstallOneFromChannelFail(c *gc.C) {
+	_, err := InstallOneFromChannel("foo", "latest/stable", RefreshBase{
 		Name:         "ubuntu",
 		Channel:      "20.04",
 		Architecture: arch.DefaultArchitecture,
 	})
 	c.Assert(err, jc.ErrorIsNil)
+}
+
+func (s *RefreshConfigSuite) TestDownloadOneFromRevisionBuild(c *gc.C) {
+	rev := 4
+	id := "foo"
+	config, err := DownloadOneFromRevision(id, rev)
+	c.Assert(err, jc.ErrorIsNil)
 
 	config = DefineInstanceKey(c, config, "foo-bar")
 
-	err = config.Ensure([]transport.RefreshResponse{{
-		InstanceKey: "foo-bar",
-	}})
+	req, _, err := config.Build()
 	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(req, gc.DeepEquals, transport.RefreshRequest{
+		Context: []transport.RefreshRequestContext{},
+		Actions: []transport.RefreshRequestAction{{
+			Action:      "download",
+			InstanceKey: "foo-bar",
+			ID:          &id,
+			Revision:    &rev,
+		}},
+		Fields: []string{"bases", "download", "id", "revision", "version", "resources", "type"},
+	})
 }
 
-func (s *RefreshConfigSuite) TestDownloadOneEnsure(c *gc.C) {
-	config, err := DownloadOne("foo", 1, "latest/stable", RefreshBase{
-		Name:         "ubuntu",
-		Channel:      "20.04",
-		Architecture: arch.DefaultArchitecture,
-	})
+func (s *RefreshConfigSuite) TestDownloadOneFromRevisionFail(c *gc.C) {
+	_, err := DownloadOneFromRevision("", 7)
+	c.Assert(err, jc.Satisfies, errors.IsNotValid)
+}
+
+func (s *RefreshConfigSuite) TestDownloadOneFromRevisionByNameBuild(c *gc.C) {
+	rev := 4
+	name := "foo"
+	config, err := DownloadOneFromRevisionByName(name, rev)
 	c.Assert(err, jc.ErrorIsNil)
 
 	config = DefineInstanceKey(c, config, "foo-bar")
 
-	err = config.Ensure([]transport.RefreshResponse{{
-		InstanceKey: "foo-bar",
-	}})
+	req, _, err := config.Build()
 	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(req, gc.DeepEquals, transport.RefreshRequest{
+		Context: []transport.RefreshRequestContext{},
+		Actions: []transport.RefreshRequestAction{{
+			Action:      "download",
+			InstanceKey: "foo-bar",
+			Name:        &name,
+			Revision:    &rev,
+		}},
+		Fields: []string{"bases", "download", "id", "revision", "version", "resources", "type"},
+	})
+}
+
+func (s *RefreshConfigSuite) TestDownloadOneFromRevisionByNameFail(c *gc.C) {
+	_, err := DownloadOneFromRevisionByName("", 7)
+	c.Assert(err, jc.Satisfies, errors.IsNotValid)
 }
 
 func (s *RefreshConfigSuite) TestDownloadOneFromChannelBuild(c *gc.C) {
@@ -689,6 +743,54 @@ func (s *RefreshConfigSuite) TestDownloadOneFromChannelBuild(c *gc.C) {
 	})
 }
 
+func (s *RefreshConfigSuite) TestDownloadOneFromChannelFail(c *gc.C) {
+	_, err := DownloadOneFromChannel("", "latest/stable", RefreshBase{
+		Name:         "ubuntu",
+		Channel:      "20.04",
+		Architecture: arch.DefaultArchitecture,
+	})
+	c.Assert(err, jc.Satisfies, errors.IsNotValid)
+}
+
+func (s *RefreshConfigSuite) TestDownloadOneFromChannelByNameBuild(c *gc.C) {
+	channel := "latest/stable"
+	name := "foo"
+	config, err := DownloadOneFromChannelByName(name, channel, RefreshBase{
+		Name:         "ubuntu",
+		Channel:      "20.04",
+		Architecture: arch.DefaultArchitecture,
+	})
+	c.Assert(err, jc.ErrorIsNil)
+
+	config = DefineInstanceKey(c, config, "foo-bar")
+
+	req, _, err := config.Build()
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(req, gc.DeepEquals, transport.RefreshRequest{
+		Context: []transport.RefreshRequestContext{},
+		Actions: []transport.RefreshRequestAction{{
+			Action:      "download",
+			InstanceKey: "foo-bar",
+			Name:        &name,
+			Channel:     &channel,
+			Base: &transport.Base{
+				Name:         "ubuntu",
+				Channel:      "20.04",
+				Architecture: arch.DefaultArchitecture,
+			},
+		}},
+	})
+}
+
+func (s *RefreshConfigSuite) TestDownloadOneFromChannelByNameFail(c *gc.C) {
+	_, err := DownloadOneFromChannel("", "latest/stable", RefreshBase{
+		Name:         "ubuntu",
+		Channel:      "20.04",
+		Architecture: arch.DefaultArchitecture,
+	})
+	c.Assert(err, jc.Satisfies, errors.IsNotValid)
+}
+
 func (s *RefreshConfigSuite) TestDownloadOneFromChannelBuildK8s(c *gc.C) {
 	channel := "latest/stable"
 	id := "foo"