Merge pull request juju#14074 from wallyworld/refactor-resources-4

juju#14074 The resources package used too many layers of abstraction. This PR simplifies the implementation of the resource opener, which fetches a resource from the store and caches in state. ## QA steps See QA steps for this previous PR juju#14049 Essentially, deploy charms with resources and run various hook commands.
addyess · May 25, 2022 · a623f79 · a623f79
2 parents 2b96c7e + 6ffb7b1
commit a623f79
Show file tree

Hide file tree

Showing 19 changed files with 459 additions and 1,229 deletions.
diff --git a/apiserver/apiserver.go b/apiserver/apiserver.go
@@ -753,8 +753,7 @@ func (srv *Server) endpoints() []apihttp.Endpoint {
 			if err != nil {
 				return nil, nil, errors.Trace(err)
 			}
-			opener, err := resource.NewResourceOpener(
-				resource.NewResourceOpenerState(st.State), srv.getResourceDownloadLimiter, tag.Id())
+			opener, err := resource.NewResourceOpener(st.State, srv.getResourceDownloadLimiter, tag.Id())
 			if err != nil {
 				return nil, nil, errors.Trace(err)
 			}

diff --git a/apiserver/facades/controller/caasapplicationprovisioner/provisioner.go b/apiserver/facades/controller/caasapplicationprovisioner/provisioner.go
@@ -104,7 +104,7 @@ func NewStateCAASApplicationProvisionerAPI(ctx facade.Context) (*APIGroup, error
 	}
 
 	newResourceOpener := func(appName string) (coreresource.Opener, error) {
-		return resource.NewResourceOpenerForApplication(resource.NewResourceOpenerState(st), appName)
+		return resource.NewResourceOpenerForApplication(st, appName)
 	}
 
 	api, err := NewCAASApplicationProvisionerAPI(

diff --git a/core/resource/unit.go b/core/resource/unit.go
diff --git a/resource/charmhub.go b/resource/charmhub.go
@@ -16,6 +16,7 @@ import (
 	"github.com/juju/juju/charmhub"
 	"github.com/juju/juju/charmhub/transport"
 	corelogger "github.com/juju/juju/core/logger"
+	"github.com/juju/juju/state"
 )
 
 type charmHubOpener struct {
@@ -37,7 +38,7 @@ func (ch *charmHubOpener) NewClient() (*ResourceRetryClient, error) {
 // chClientState represents a state which can provide a model to create a
 // CharmHub client.
 type chClientState interface {
-	Model() (Model, error)
+	Model() (*state.Model, error)
 }
 
 func newCharmHubClient(st chClientState) (ResourceGetter, error) {

diff --git a/resource/export_test.go b/resource/export_test.go
@@ -6,7 +6,10 @@ package resource
 import (
 	"time"
 
+	"github.com/juju/charm/v8"
 	"github.com/juju/names/v4"
+
+	"github.com/juju/juju/state"
 )
 
 func NewCSRetryClientForTest(client ResourceGetter) *ResourceRetryClient {
@@ -24,27 +27,32 @@ func NewCharmHubClientForTest(cl CharmHub, logger Logger) *CharmHubClient {
 }
 
 func NewResourceRetryClientForTest(cl ResourceGetter) *ResourceRetryClient {
-	return newRetryClient(cl)
+	client := newRetryClient(cl)
+	client.retryArgs.Delay = time.Millisecond
+	return client
 }
 
 func NewResourceOpenerForTest(
-	st ResourceOpenerState,
 	res Resources,
 	tag names.Tag,
-	unit Unit,
-	application Application,
-	fn func(st ResourceOpenerState) ResourceRetryClientGetter,
-	maxRequests int,
+	unitName string,
+	appName string,
+	charmURL *charm.URL,
+	charmOrigin state.CharmOrigin,
+	resourceClient ResourceGetter,
+	resourceDownloadLimiter ResourceDownloadLock,
 ) *ResourceOpener {
 	return &ResourceOpener{
-		st:                st,
-		res:               res,
-		userID:            tag,
-		unit:              unit,
-		application:       application,
-		newResourceOpener: fn,
+		modelUUID:      "uuid",
+		resourceCache:  res,
+		user:           tag,
+		unitName:       unitName,
+		appName:        appName,
+		charmURL:       charmURL,
+		charmOrigin:    charmOrigin,
+		resourceClient: resourceClient,
 		resourceDownloadLimiterFunc: func() ResourceDownloadLock {
-			return NewResourceDownloadLimiter(maxRequests, 0)
+			return resourceDownloadLimiter
 		},
 	}
 }
diff --git a/resource/interfaces.go b/resource/interfaces.go
@@ -8,76 +8,24 @@ import (
 	"io"
 	"net/url"
 
-	"github.com/juju/charm/v8"
 	charmresource "github.com/juju/charm/v8/resource"
-	"github.com/juju/names/v4"
 
 	"github.com/juju/juju/charmhub"
 	"github.com/juju/juju/charmhub/transport"
-	"github.com/juju/juju/controller"
 	"github.com/juju/juju/core/resource"
-	"github.com/juju/juju/environs/config"
-	corestate "github.com/juju/juju/state"
+	"github.com/juju/juju/state"
 )
 
-// ResourceOpenerState represents methods from state required to implement
-// a resource Opener.
-type ResourceOpenerState interface {
-	// required for csClientState
-	Charm(*charm.URL) (*corestate.Charm, error)
-	ControllerConfig() (controller.Config, error)
-
-	// required for the chClientState
-	Model() (Model, error)
-
-	// required for NewResourceOpener and OpenResource
-	Resources() (Resources, error)
-	Unit(string) (Unit, error)
-	Application(string) (Application, error)
-	ModelUUID() string
-}
-
-// Model represents model methods required to open a resource.
-type Model interface {
-	Config() (*config.Config, error)
-}
-
-// Unit represents unit methods required to open a resource.
-type Unit interface {
-	resource.Unit
-
-	Application() (Application, error)
-	Tag() names.Tag
-}
-
-// Application represents application methods required to open a resource.
-type Application interface {
-	CharmOrigin() *corestate.CharmOrigin
-	CharmURL() (*charm.URL, bool)
-	Name() string
-	Tag() names.Tag
-}
-
-// Resources represents the methods used by resourceCache from state.Resources .
+// Resources represents the methods used by the resource opener from state.Resources.
 type Resources interface {
 	// GetResource returns the identified resource.
 	GetResource(applicationID, name string) (resource.Resource, error)
 	// OpenResource returns the metadata for a resource and a reader for the resource.
 	OpenResource(applicationID, name string) (resource.Resource, io.ReadCloser, error)
 	// OpenResourceForUniter returns the metadata for a resource and a reader for the resource.
-	OpenResourceForUniter(unit resource.Unit, name string) (resource.Resource, io.ReadCloser, error)
+	OpenResourceForUniter(appName, unitName, resName string) (resource.Resource, io.ReadCloser, error)
 	// SetResource adds the resource to blob storage and updates the metadata.
-	SetResource(applicationID, userID string, res charmresource.Resource, r io.Reader, _ corestate.IncrementCharmModifiedVersionType) (resource.Resource, error)
-}
-
-// ResourceRetryClientGetterFn allows the creation of ResourceRetryClientGetter
-// from a given state.
-type ResourceRetryClientGetterFn func(st ResourceOpenerState) ResourceRetryClientGetter
-
-// ResourceRetryClientGetter defines an interface for creating a new resource
-// retry clients.
-type ResourceRetryClientGetter interface {
-	NewClient() (*ResourceRetryClient, error)
+	SetResource(applicationID, userID string, res charmresource.Resource, r io.Reader, _ state.IncrementCharmModifiedVersionType) (resource.Resource, error)
 }
 
 // ResourceGetter provides the functionality for getting a resource file.

diff --git a/resource/mocks/cache_mock.go b/resource/mocks/cache_mock.go