Update application CharmURL signature.

Return the charm url as a string pointer, like it's saved as. No need to translate it to an actual *charm.URL unless it's required that way. More changes along this line are coming, so a few contortions have been temporaily made.
addyess · Jun 9, 2022 · 1a5255f · 1a5255f
1 parent 7c13fb2
commit 1a5255f
Show file tree

Hide file tree

Showing 39 changed files with 214 additions and 122 deletions.
diff --git a/apiserver/common/crossmodel/interface.go b/apiserver/common/crossmodel/interface.go
@@ -9,13 +9,14 @@ import (
 	"github.com/juju/charm/v8"
 	"gopkg.in/macaroon.v2"
 
+	"github.com/juju/names/v4"
+
 	"github.com/juju/juju/core/crossmodel"
 	"github.com/juju/juju/core/network"
 	"github.com/juju/juju/core/network/firewall"
 	"github.com/juju/juju/core/permission"
 	"github.com/juju/juju/core/status"
 	"github.com/juju/juju/state"
-	"github.com/juju/names/v4"
 )
 
 type Backend interface {
@@ -211,9 +212,10 @@ type Application interface {
 	// charm even if they are in an error state.
 	Charm() (ch Charm, force bool, err error)
 
-	// CharmURL returns the application's charm URL, and whether units should upgrade
-	// to the charm with that URL even if they are in an error state.
-	CharmURL() (curl *charm.URL, force bool)
+	// CharmURL returns a string representation the application's charm URL,
+	// and whether units should upgrade to the charm with that URL even if
+	// they are in an error state.
+	CharmURL() (curl *string, force bool)
 
 	// EndpointBindings returns the Bindings object for this application.
 	EndpointBindings() (Bindings, error)

diff --git a/apiserver/facades/agent/instancemutater/instancemutater.go b/apiserver/facades/agent/instancemutater/instancemutater.go
@@ -4,6 +4,7 @@
 package instancemutater
 
 import (
+	"github.com/juju/charm/v8"
 	"github.com/juju/errors"
 	"github.com/juju/loggo"
 	"github.com/juju/names/v4"
@@ -339,7 +340,12 @@ func (api *InstanceMutaterAPI) machineLXDProfileInfo(m Machine) (lxdProfileInfo,
 			changeResults[i].Error = apiservererrors.ServerError(err)
 			continue
 		}
-		chURL := app.CharmURL()
+		cURL := app.CharmURL()
+		chURL, err := charm.ParseURL(*cURL)
+		if err != nil {
+			changeResults[i].Error = apiservererrors.ServerError(err)
+			continue
+		}
 		ch, err := api.st.Charm(chURL)
 		if err != nil {
 			changeResults[i].Error = apiservererrors.ServerError(err)
@@ -356,7 +362,7 @@ func (api *InstanceMutaterAPI) machineLXDProfileInfo(m Machine) (lxdProfileInfo,
 		}
 		changeResults[i] = params.ProfileInfoResult{
 			ApplicationName: appName,
-			Revision:        chURL.Revision,
+			Revision:        ch.Revision(),
 			Profile:         normalised,
 		}
 	}

diff --git a/apiserver/facades/agent/instancemutater/instancemutater_test.go b/apiserver/facades/agent/instancemutater/instancemutater_test.go
@@ -12,7 +12,6 @@ import (
 	"github.com/juju/errors"
 	"github.com/juju/names/v4"
 	coretesting "github.com/juju/testing"
-	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 
 	apiservererrors "github.com/juju/juju/apiserver/errors"
@@ -260,7 +259,7 @@ func (s *InstanceMutaterAPICharmProfilingInfoSuite) TestCharmProfilingInfo(c *gc
 	s.expectInstanceId("0")
 	s.expectUnits(1)
 	s.expectCharmProfiles()
-	s.expectProfileExtraction(c)
+	s.expectProfileExtraction()
 	s.expectName()
 	facade := s.facadeAPIForScenario(c)
 
@@ -302,8 +301,8 @@ func (s *InstanceMutaterAPICharmProfilingInfoSuite) TestCharmProfilingInfoWithNo
 	s.expectInstanceId("0")
 	s.expectUnits(2)
 	s.expectCharmProfiles()
-	s.expectProfileExtraction(c)
-	s.expectProfileExtractionWithEmpty(c)
+	s.expectProfileExtraction()
+	s.expectProfileExtractionWithEmpty()
 	s.expectName()
 	facade := s.facadeAPIForScenario(c)
 
@@ -393,18 +392,19 @@ func (s *InstanceMutaterAPICharmProfilingInfoSuite) expectCharmProfiles() {
 	machineExp.CharmProfiles().Return([]string{"charm-app-0"}, nil)
 }
 
-func (s *InstanceMutaterAPICharmProfilingInfoSuite) expectProfileExtraction(c *gc.C) {
+func (s *InstanceMutaterAPICharmProfilingInfoSuite) expectProfileExtraction() {
 	appExp := s.application.EXPECT()
 	charmExp := s.charm.EXPECT()
 	stateExp := s.state.EXPECT()
 	unitExp := s.unit.EXPECT()
 
 	unitExp.ApplicationName().Return("foo")
 	stateExp.Application("foo").Return(s.application, nil)
-	chURL, err := charm.ParseURL("cs:app-0")
-	c.Assert(err, jc.ErrorIsNil)
-	appExp.CharmURL().Return(chURL)
+	chURLStr := "cs:app-0"
+	appExp.CharmURL().Return(&chURLStr)
+	chURL := charm.MustParseURL(chURLStr)
 	stateExp.Charm(chURL).Return(s.charm, nil)
+	charmExp.Revision().Return(chURL.Revision)
 	charmExp.LXDProfile().Return(lxdprofile.Profile{
 		Config: map[string]string{
 			"security.nesting": "true",
@@ -418,18 +418,19 @@ func (s *InstanceMutaterAPICharmProfilingInfoSuite) expectProfileExtraction(c *g
 	})
 }
 
-func (s *InstanceMutaterAPICharmProfilingInfoSuite) expectProfileExtractionWithEmpty(c *gc.C) {
+func (s *InstanceMutaterAPICharmProfilingInfoSuite) expectProfileExtractionWithEmpty() {
 	appExp := s.application.EXPECT()
 	charmExp := s.charm.EXPECT()
 	stateExp := s.state.EXPECT()
 	unitExp := s.unit.EXPECT()
 
 	unitExp.ApplicationName().Return("foo")
 	stateExp.Application("foo").Return(s.application, nil)
-	chURL, err := charm.ParseURL("cs:app-0")
-	c.Assert(err, jc.ErrorIsNil)
-	appExp.CharmURL().Return(chURL)
+	chURLStr := "cs:app-0"
+	appExp.CharmURL().Return(&chURLStr)
+	chURL := charm.MustParseURL(chURLStr)
 	stateExp.Charm(chURL).Return(s.charm, nil)
+	charmExp.Revision().Return(chURL.Revision)
 	charmExp.LXDProfile().Return(lxdprofile.Profile{})
 }
 

diff --git a/apiserver/facades/agent/instancemutater/interface.go b/apiserver/facades/agent/instancemutater/interface.go
@@ -60,9 +60,10 @@ type Unit interface {
 // Charm represents point of use methods from the state Charm object.
 type Charm interface {
 	LXDProfile() lxdprofile.Profile
+	Revision() int
 }
 
 // Application represents point of use methods from the state Application object.
 type Application interface {
-	CharmURL() *charm.URL
+	CharmURL() *string
 }
diff --git a/apiserver/facades/agent/instancemutater/lxdprofilewatcher.go b/apiserver/facades/agent/instancemutater/lxdprofilewatcher.go
@@ -226,9 +226,13 @@ func (w *machineLXDProfileWatcher) init() error {
 			}
 		}
 
-		chURL := app.CharmURL()
+		cURL := app.CharmURL()
+		chURL, err := charm.ParseURL(*cURL)
+		if err != nil {
+			return err
+		}
 		info := appInfo{
-			charmURL: chURL.String(),
+			charmURL: *cURL,
 			units:    set.NewStrings(unitName),
 		}
 
@@ -271,19 +275,24 @@ func (w *machineLXDProfileWatcher) applicationCharmURLChange(appName string) err
 	} else if err != nil {
 		return errors.Trace(err)
 	}
-	chURL := app.CharmURL()
+
 	info, ok := w.applications[appName]
 	if ok {
+		cURL := app.CharmURL()
 		// Have we already seen this charm URL change?
-		if info.charmURL == chURL.String() {
+		if info.charmURL == *cURL {
 			return nil
 		}
+		chURL, err := charm.ParseURL(*cURL)
+		if err != nil {
+			return errors.Trace(err)
+		}
 		ch, err := w.backend.Charm(chURL)
 		if errors.IsNotFound(err) {
-			logger.Debugf("not watching %s with removed charm %s on machine-%s", appName, chURL, w.machine.Id())
+			logger.Debugf("not watching %s with removed charm %s on machine-%s", appName, *cURL, w.machine.Id())
 			return nil
 		} else if err != nil {
-			return errors.Annotatef(err, "error getting charm %s to evaluate for lxd profile notification", chURL)
+			return errors.Annotatef(err, "error getting charm %s to evaluate for lxd profile notification", *cURL)
 		}
 
 		// notify if:
@@ -298,7 +307,7 @@ func (w *machineLXDProfileWatcher) applicationCharmURLChange(appName string) err
 		}
 
 		info.charmProfile = lxdProfile
-		info.charmURL = chURL.String()
+		info.charmURL = *cURL
 		w.applications[appName] = info
 	} else {
 		logger.Tracef("not watching %s on machine-%s", appName, w.machine.Id())
@@ -454,7 +463,11 @@ func (w *machineLXDProfileWatcher) add(unit Unit) (bool, error) {
 			} else if err != nil {
 				return false, errors.Annotatef(err, "failed to get application %s for machine-%s", appName, w.machine.Id())
 			}
-			curl = app.CharmURL()
+			cURL := app.CharmURL()
+			curl, err = charm.ParseURL(*cURL)
+			if err != nil {
+				return false, errors.Annotatef(err, "application charm url")
+			}
 		}
 
 		ch, err := w.backend.Charm(curl)

diff --git a/apiserver/facades/agent/instancemutater/lxdprofilewatcher_test.go b/apiserver/facades/agent/instancemutater/lxdprofilewatcher_test.go
@@ -330,9 +330,9 @@ func (s *lxdProfileWatcherSuite) TestMachineLXDProfileWatcherAppChangeCharmURLNo
 	s.wc0.AssertOneChange()
 
 	s.state.EXPECT().Application("foo").Return(s.app, nil)
-	curl := charm.MustParseURL("ch:name-me-3")
-	s.app.EXPECT().CharmURL().Return(curl)
-	s.state.EXPECT().Charm(curl).Return(nil, errors.NotFoundf(""))
+	curl := "ch:name-me-3"
+	s.app.EXPECT().CharmURL().Return(&curl)
+	s.state.EXPECT().Charm(charm.MustParseURL(curl)).Return(nil, errors.NotFoundf(""))
 
 	s.appChanges <- []string{"foo"}
 	s.wc0.AssertNoChange()
@@ -394,9 +394,9 @@ func (s *lxdProfileWatcherSuite) updateCharmForMachineLXDProfileWatcher(rev stri
 	} else {
 		s.charm.EXPECT().LXDProfile().Return(lxdprofile.Profile{})
 	}
-	chURL := charm.MustParseURL(curl)
 	s.state.EXPECT().Application("foo").Return(s.app, nil)
-	s.app.EXPECT().CharmURL().Return(chURL)
+	s.app.EXPECT().CharmURL().Return(&curl)
+	chURL := charm.MustParseURL(curl)
 	s.state.EXPECT().Charm(chURL).Return(s.charm, nil)
 	s.charmChanges <- []string{curl}
 	s.appChanges <- []string{"foo"}
@@ -451,14 +451,15 @@ func (s *lxdProfileWatcherSuite) setupScenario(startEmpty, withProfile bool) {
 	s.unit.EXPECT().ApplicationName().AnyTimes().Return("foo")
 	s.unit.EXPECT().Name().AnyTimes().Return("foo/0")
 
-	curl := charm.MustParseURL("ch:name-me")
+	curlStr := "ch:name-me"
+	curl := charm.MustParseURL(curlStr)
 	s.state.EXPECT().Charm(curl).Return(s.charm, nil)
 	if startEmpty {
 		s.machine0.EXPECT().Units().Return(nil, nil)
 	} else {
 		s.machine0.EXPECT().Units().Return([]instancemutater.Unit{s.unit}, nil)
 		s.unit.EXPECT().Application().Return(s.app, nil)
-		s.app.EXPECT().CharmURL().Return(curl)
+		s.app.EXPECT().CharmURL().Return(&curlStr)
 	}
 
 	if withProfile {

diff --git a/apiserver/facades/agent/instancemutater/mocks/instancemutater_mock.go b/apiserver/facades/agent/instancemutater/mocks/instancemutater_mock.go
diff --git a/apiserver/facades/agent/instancemutater/shim.go b/apiserver/facades/agent/instancemutater/shim.go
@@ -103,7 +103,7 @@ type applicationShim struct {
 	*state.Application
 }
 
-func (a *applicationShim) CharmURL() *charm.URL {
+func (a *applicationShim) CharmURL() *string {
 	curl, _ := a.Application.CharmURL()
 	return curl
 }

diff --git a/apiserver/facades/agent/uniter/uniter.go b/apiserver/facades/agent/uniter/uniter.go
@@ -692,22 +692,28 @@ func (u *UniterAPI) CharmURL(args params.Entities) (params.StringBoolResults, er
 			var unitOrApplication state.Entity
 			unitOrApplication, err = u.st.FindEntity(tag)
 			if err == nil {
+				// TODO (hmlanigan) 2022-06-08
+				// cURL can be a string pointer once unit.CharmURL()
+				// returns a string pointer as well.
 				var cURL *charm.URL
-				var ok bool
+				var force bool
 
 				switch entity := unitOrApplication.(type) {
 				case *state.Application:
-					cURL, ok = entity.CharmURL()
+					var cURLStr *string
+					cURLStr, force = entity.CharmURL()
+					cURL, err = charm.ParseURL(*cURLStr)
 				case *state.Unit:
 					cURL, err = entity.CharmURL()
+					// The force value is not actually used on the uniter's unit api.
 					if cURL != nil {
-						ok = true
+						force = true
 					}
 				}
 
 				if cURL != nil {
 					result.Results[i].Result = cURL.String()
-					result.Results[i].Ok = ok
+					result.Results[i].Ok = force
 				}
 			}
 		}

diff --git a/apiserver/facades/agent/uniter/uniter_test.go b/apiserver/facades/agent/uniter/uniter_test.go
@@ -880,8 +880,8 @@ func (s *uniterSuite) TestCharmURL(c *gc.C) {
 	c.Assert(curl, gc.DeepEquals, s.wpCharm.URL())
 
 	// Make sure wordpress application's charm is what we expect.
-	curl, force := s.wordpress.CharmURL()
-	c.Assert(curl, gc.DeepEquals, s.wpCharm.URL())
+	curlStr, force := s.wordpress.CharmURL()
+	c.Assert(*curlStr, gc.DeepEquals, s.wpCharm.URL().String())
 	c.Assert(force, jc.IsFalse)
 
 	args := params.Entities{Entities: []params.Entity{
@@ -938,7 +938,7 @@ func (s *uniterSuite) TestSetCharmURL(c *gc.C) {
 	charmURL, err := s.wordpressUnit.CharmURL()
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(charmURL, gc.NotNil)
-	c.Assert(charmURL.String(), gc.Equals, s.wpCharm.String())
+	c.Assert(*charmURL, gc.Equals, s.wpCharm.String())
 }
 
 func (s *uniterSuite) TestWorkloadVersion(c *gc.C) {

diff --git a/apiserver/facades/client/application/application.go b/apiserver/facades/client/application/application.go
@@ -1403,7 +1403,7 @@ func (api *APIBase) GetCharmURL(args params.ApplicationGet) (params.StringResult
 		return params.StringResult{}, errors.Trace(err)
 	}
 	charmURL, _ := oneApplication.CharmURL()
-	return params.StringResult{Result: charmURL.String()}, nil
+	return params.StringResult{Result: *charmURL}, nil
 }
 
 // GetCharmURLOrigin isn't on the V12 API.
@@ -1420,7 +1420,11 @@ func (api *APIBase) GetCharmURLOrigin(args params.ApplicationGet) (params.CharmU
 		return params.CharmURLOriginResult{Error: apiservererrors.ServerError(err)}, nil
 	}
 	charmURL, _ := oneApplication.CharmURL()
-	result := params.CharmURLOriginResult{URL: charmURL.String()}
+	if charmURL == nil {
+		err := errors.NotValidf("application charm url")
+		return params.CharmURLOriginResult{Error: apiservererrors.ServerError(err)}, nil
+	}
+	result := params.CharmURLOriginResult{URL: *charmURL}
 	chOrigin := oneApplication.CharmOrigin()
 	if chOrigin == nil {
 		result.Error = apiservererrors.ServerError(errors.NotFoundf("charm origin for %q", args.ApplicationName))
@@ -3074,6 +3078,9 @@ func (api *APIBase) unitResultForUnit(unit Unit) (*params.UnitResult, error) {
 		return nil, err
 	}
 	curl, _ := app.CharmURL()
+	if curl == nil {
+		return nil, errors.NotValidf("application charm url")
+	}
 	machineId, _ := unit.AssignedMachineId()
 	workloadVersion, err := unit.WorkloadVersion()
 	if err != nil {
@@ -3084,7 +3091,7 @@ func (api *APIBase) unitResultForUnit(unit Unit) (*params.UnitResult, error) {
 		Tag:             unit.Tag().String(),
 		WorkloadVersion: workloadVersion,
 		Machine:         machineId,
-		Charm:           curl.String(),
+		Charm:           *curl,
 		Life:            unit.Life().String(),
 	}
 	if machineId != "" {