Merge pull request juju#10903 from hmlanigan/use-spaceinfos

juju#10903 ## Description of change Replace SpaceInfosByID(), SpaceNamesByID() and SpaceIDsByName() with AllSpaceInfos(). Reduces the number of Add methods attached to SpaceInfo{} to facilitate use in replacing the mentioned functions: ContainsName(), ContainsID(), and Difference(). ## QA steps 1. deploy a charm with bindings 2. upgrade juju
hopkings2008 · Nov 14, 2019 · f5a4e10 · f5a4e10
2 parents aed3be2 + 7f12c54
commit f5a4e10
Show file tree

Hide file tree

Showing 22 changed files with 271 additions and 305 deletions.
diff --git a/apiserver/facades/client/application/mock_test.go b/apiserver/facades/client/application/mock_test.go
@@ -433,11 +433,7 @@ func (m *mockBackend) Machine(id string) (application.Machine, error) {
 	return nil, errors.NotFoundf("machine %q", id)
 }
 
-func (m *mockBackend) SpaceIDsByName() (map[string]string, error) {
-	return nil, nil
-}
-
-func (m *mockBackend) SpaceNamesByID() (map[string]string, error) {
+func (m *mockBackend) AllSpaceInfos() (network.SpaceInfos, error) {
 	return nil, nil
 }
 

diff --git a/apiserver/facades/client/bundle/mock_test.go b/apiserver/facades/client/bundle/mock_test.go
@@ -8,6 +8,7 @@ import (
 	"github.com/juju/testing"
 
 	"github.com/juju/juju/apiserver/facades/client/bundle"
+	"github.com/juju/juju/core/network"
 	"github.com/juju/juju/state"
 )
 
@@ -39,16 +40,14 @@ func (m *mockState) GetExportConfig() state.ExportConfig {
 	}
 }
 
-func (m *mockState) SpaceNamesByID() (map[string]string, error) {
-	return m.Spaces, nil
-}
-
-func (m *mockState) SpaceIDsByName() (map[string]string, error) {
-	idsByName := make(map[string]string, len(m.Spaces))
-	for k, v := range m.Spaces {
-		idsByName[v] = k
+func (m *mockState) AllSpaceInfos() (network.SpaceInfos, error) {
+	result := make(network.SpaceInfos, len(m.Spaces))
+	i := 0
+	for id, name := range m.Spaces {
+		result[i] = network.SpaceInfo{ID: id, Name: network.SpaceName(name)}
+		i += 1
 	}
-	return idsByName, nil
+	return result, nil
 }
 
 func (m *mockState) Space(_ string) (*state.Space, error) {

diff --git a/apiserver/facades/client/highavailability/highavailability.go b/apiserver/facades/client/highavailability/highavailability.go
@@ -248,20 +248,20 @@ func validatePlacementForSpaces(st *state.State, spaceNames *[]string, placement
 			return errors.Annotate(err, "retrieving machine")
 		}
 
-		spaceLookup, err := st.SpaceIDsByName()
+		spaceInfos, err := st.AllSpaceInfos()
 		if err != nil {
 			return errors.Trace(err)
 		}
 
 		for _, name := range *spaceNames {
-			spaceID, ok := spaceLookup[name]
-			if !ok {
+			spaceInfo := spaceInfos.GetByName(name)
+			if spaceInfo == nil {
 				return errors.NotFoundf("space with name %q", name)
 			}
 
 			inSpace := false
 			for _, addr := range m.Addresses() {
-				if addr.SpaceID == spaceID {
+				if addr.SpaceID == spaceInfo.ID {
 					inSpace = true
 					break
 				}

diff --git a/apiserver/facades/controller/caasunitprovisioner/mock_test.go b/apiserver/facades/controller/caasunitprovisioner/mock_test.go
@@ -85,14 +85,9 @@ func (st *mockState) ResolveConstraints(cons constraints.Value) (constraints.Val
 	return cons, nil
 }
 
-func (st *mockState) SpaceIDsByName() (map[string]string, error) {
-	st.MethodCall(st, "SpaceIDsByName")
-	return map[string]string{}, nil
-}
-
-func (st *mockState) SpaceInfosByID() (map[string]network.SpaceInfo, error) {
-	st.MethodCall(st, "SpaceInfosByID")
-	return map[string]network.SpaceInfo{}, nil
+func (m *mockState) AllSpaceInfos() (network.SpaceInfos, error) {
+	m.MethodCall(m, "AllSpaceInfos")
+	return network.SpaceInfos{}, nil
 }
 
 type mockModel struct {

diff --git a/apiserver/facades/controller/instancepoller/instancepoller_test.go b/apiserver/facades/controller/instancepoller/instancepoller_test.go
@@ -432,7 +432,7 @@ func (s *InstancePollerSuite) TestProviderAddressesSuccess(c *gc.C) {
 
 	s.st.CheckFindEntityCall(c, 0, "1")
 	s.st.CheckCall(c, 1, "ProviderAddresses")
-	s.st.CheckCall(c, 2, "SpaceInfosByName")
+	s.st.CheckCall(c, 2, "AllSpaceInfos")
 	s.st.CheckFindEntityCall(c, 3, "2")
 	s.st.CheckCall(c, 4, "ProviderAddresses")
 	s.st.CheckFindEntityCall(c, 5, "42")
@@ -488,7 +488,7 @@ func (s *InstancePollerSuite) TestSetProviderAddressesSuccess(c *gc.C) {
 	s.st.CheckFindEntityCall(c, 0, "1")
 	s.st.CheckSetProviderAddressesCall(c, 1, []network.SpaceAddress{})
 	s.st.CheckFindEntityCall(c, 2, "2")
-	s.st.CheckCall(c, 3, "SpaceIDsByName")
+	s.st.CheckCall(c, 3, "AllSpaceInfos")
 	s.st.CheckSetProviderAddressesCall(c, 4, newAddrs)
 	s.st.CheckFindEntityCall(c, 5, "42")
 
@@ -526,7 +526,7 @@ func (s *InstancePollerSuite) TestSetProviderAddressesFailure(c *gc.C) {
 
 	s.st.CheckFindEntityCall(c, 0, "1")
 	s.st.CheckFindEntityCall(c, 1, "2")
-	s.st.CheckCall(c, 2, "SpaceIDsByName")
+	s.st.CheckCall(c, 2, "AllSpaceInfos")
 	s.st.CheckSetProviderAddressesCall(c, 3, newAddrs)
 	s.st.CheckFindEntityCall(c, 4, "3")
 

diff --git a/apiserver/facades/controller/instancepoller/mock_test.go b/apiserver/facades/controller/instancepoller/mock_test.go
@@ -200,18 +200,11 @@ func (m *mockState) Machine(id string) (instancepoller.StateMachine, error) {
 	return machine, nil
 }
 
-// SpaceIDsByName implements network.SpaceLookup.
+// AllSpaceInfos implements network.AllSpaceInfos.
 // This method never throws an error.
-func (m *mockState) SpaceIDsByName() (map[string]string, error) {
-	m.MethodCall(m, "SpaceIDsByName")
-	return map[string]string{}, nil
-}
-
-// SpaceInfosByID implements network.SpaceLookup.
-// This method never throws an error.
-func (m *mockState) SpaceInfosByID() (map[string]network.SpaceInfo, error) {
-	m.MethodCall(m, "SpaceInfosByName")
-	return map[string]network.SpaceInfo{}, nil
+func (m *mockState) AllSpaceInfos() (network.SpaceInfos, error) {
+	m.MethodCall(m, "AllSpaceInfos")
+	return network.SpaceInfos{}, nil
 }
 
 // StartSync implements statetesting.SyncStarter, so mockState can be

diff --git a/core/network/address.go b/core/network/address.go
@@ -374,10 +374,10 @@ func (pas ProviderAddresses) ToSpaceAddresses(lookup SpaceLookup) (SpaceAddresse
 		return nil, nil
 	}
 
-	var idFor map[string]string
+	var spaceInfos SpaceInfos
 	if len(pas) > 0 {
 		var err error
-		if idFor, err = lookup.SpaceIDsByName(); err != nil {
+		if spaceInfos, err = lookup.AllSpaceInfos(); err != nil {
 			return nil, errors.Trace(err)
 		}
 	}
@@ -386,11 +386,11 @@ func (pas ProviderAddresses) ToSpaceAddresses(lookup SpaceLookup) (SpaceAddresse
 	for i, pa := range pas {
 		sas[i] = SpaceAddress{MachineAddress: pa.MachineAddress}
 		if pa.SpaceName != "" {
-			id, ok := idFor[string(pa.SpaceName)]
-			if !ok {
+			info := spaceInfos.GetByName(string(pa.SpaceName))
+			if info == nil {
 				return nil, errors.NotFoundf("space with name %q", pa.SpaceName)
 			}
-			sas[i].SpaceID = id
+			sas[i].SpaceID = info.ID
 		}
 	}
 	return sas, nil
@@ -477,10 +477,10 @@ func (sas SpaceAddresses) ToProviderAddresses(lookup SpaceLookup) (ProviderAddre
 		return nil, nil
 	}
 
-	var infoFor map[string]SpaceInfo
+	var spaces SpaceInfos
 	if len(sas) > 0 {
 		var err error
-		if infoFor, err = lookup.SpaceInfosByID(); err != nil {
+		if spaces, err = lookup.AllSpaceInfos(); err != nil {
 			return nil, errors.Trace(err)
 		}
 	}
@@ -489,8 +489,8 @@ func (sas SpaceAddresses) ToProviderAddresses(lookup SpaceLookup) (ProviderAddre
 	for i, sa := range sas {
 		pas[i] = ProviderAddress{MachineAddress: sa.MachineAddress}
 		if sa.SpaceID != "" {
-			info, ok := infoFor[sa.SpaceID]
-			if !ok {
+			info := spaces.GetByID(sa.SpaceID)
+			if info == nil {
 				return nil, errors.NotFoundf("space with ID %q", sa.SpaceID)
 			}
 			pas[i].SpaceName = info.Name

diff --git a/core/network/address_test.go b/core/network/address_test.go
@@ -796,17 +796,10 @@ type stubLookup struct{}
 
 var _ network.SpaceLookup = stubLookup{}
 
-func (s stubLookup) SpaceIDsByName() (map[string]string, error) {
-	return map[string]string{
-		"space-one": "1",
-		"space-two": "2",
-	}, nil
-}
-
-func (s stubLookup) SpaceInfosByID() (map[string]network.SpaceInfo, error) {
-	return map[string]network.SpaceInfo{
-		"1": {ID: "1", Name: "space-one", ProviderId: "p1"},
-		"2": {ID: "2", Name: "space-two"},
+func (s stubLookup) AllSpaceInfos() (network.SpaceInfos, error) {
+	return network.SpaceInfos{
+		{ID: "1", Name: "space-one", ProviderId: "p1"},
+		{ID: "2", Name: "space-two"},
 	}, nil
 }
 

diff --git a/core/network/hostport.go b/core/network/hostport.go
@@ -325,10 +325,10 @@ func (hps SpaceHostPorts) ToProviderHostPorts(lookup SpaceLookup) (ProviderHostP
 		return nil, nil
 	}
 
-	var infoFor map[string]SpaceInfo
+	var spaces SpaceInfos
 	if len(hps) > 0 {
 		var err error
-		if infoFor, err = lookup.SpaceInfosByID(); err != nil {
+		if spaces, err = lookup.AllSpaceInfos(); err != nil {
 			return nil, errors.Trace(err)
 		}
 	}
@@ -341,8 +341,8 @@ func (hps SpaceHostPorts) ToProviderHostPorts(lookup SpaceLookup) (ProviderHostP
 		}
 
 		if hp.SpaceID != "" {
-			info, ok := infoFor[hp.SpaceID]
-			if !ok {
+			info := spaces.GetByID(hp.SpaceID)
+			if info == nil {
 				return nil, errors.NotFoundf("space with ID %q", hp.SpaceID)
 			}
 			pHPs[i].SpaceName = info.Name

diff --git a/core/network/space.go b/core/network/space.go
@@ -3,7 +3,10 @@
 
 package network
 
-import "strings"
+import (
+	"fmt"
+	"strings"
+)
 
 const (
 	// AlphaSpaceId is the ID of the alpha network space.
@@ -15,11 +18,10 @@ const (
 	AlphaSpaceName = "alpha"
 )
 
-// SpaceLookup describes methods for acquiring lookups that
-// will translate space IDs to space names and vice versa.
+// SpaceLookup describes methods for acquiring SpaceInfos
+// to translate space IDs to space names and vice versa.
 type SpaceLookup interface {
-	SpaceIDsByName() (map[string]string, error)
-	SpaceInfosByID() (map[string]SpaceInfo, error)
+	AllSpaceInfos() (SpaceInfos, error)
 }
 
 // SpaceName is the name of a network space.
@@ -45,9 +47,17 @@ type SpaceInfo struct {
 // SpaceInfos is a collection of spaces.
 type SpaceInfos []SpaceInfo
 
-// String returns the comma-delimited names of the spaces in the collection.
+// String returns returns a quoted, comma-delimited names of the spaces in the
+// collection, or <none> if the collection is empty.
 func (s SpaceInfos) String() string {
-	return strings.Join(s.Names(), ", ")
+	if len(s) == 0 {
+		return "<none>"
+	}
+	names := make([]string, len(s))
+	for i, v := range s {
+		names[i] = fmt.Sprintf("%q", string(v.Name))
+	}
+	return strings.Join(names, ", ")
 }
 
 // Names returns a string slice with each of the space names in the collection.
@@ -89,3 +99,28 @@ func (s SpaceInfos) GetByName(name string) *SpaceInfo {
 	}
 	return nil
 }
+
+// ContainsID returns true if the collection contains a
+// space with the given ID.
+func (s SpaceInfos) ContainsID(id string) bool {
+	return s.GetByID(id) != nil
+}
+
+// ContainsName returns true if the collection contains a
+// space with the given name.
+func (s SpaceInfos) ContainsName(name string) bool {
+	return s.GetByName(name) != nil
+}
+
+// Minus returns a new SpaceInfos representing all the
+// values in the target that are not in the parameter. Value
+// matching is done by ID.
+func (s SpaceInfos) Minus(other SpaceInfos) SpaceInfos {
+	result := make(SpaceInfos, 0)
+	for _, value := range s {
+		if !other.ContainsID(value.ID) {
+			result = append(result, value)
+		}
+	}
+	return result
+}
diff --git a/core/network/space_test.go b/core/network/space_test.go
@@ -5,45 +5,63 @@ package network_test
 
 import (
 	"github.com/juju/testing"
+	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 
 	"github.com/juju/juju/core/network"
 )
 
 type spaceSuite struct {
 	testing.IsolationSuite
+
+	spaces network.SpaceInfos
 }
 
 var _ = gc.Suite(&spaceSuite{})
 
-func (*spaceSuite) TestString(c *gc.C) {
-	result := network.SpaceInfos{
-		{Name: "space1"},
-		{Name: "space2"},
-		{Name: "space3"},
-	}.String()
+func (s *spaceSuite) SetUpTest(c *gc.C) {
+	s.spaces = network.SpaceInfos{
+		{ID: "1", Name: "space1"},
+		{ID: "2", Name: "space2"},
+		{ID: "3", Name: "space3"},
+	}
+}
 
-	c.Assert(result, gc.Equals, "space1, space2, space3")
+func (s *spaceSuite) TestString(c *gc.C) {
+	result := s.spaces.String()
+	c.Assert(result, gc.Equals, `"space1", "space2", "space3"`)
 }
 
-func (*spaceSuite) TestGetByName(c *gc.C) {
-	spaces := network.SpaceInfos{
-		{Name: "space1"},
-		{Name: "space2"},
-		{Name: "space3"},
-	}
+func (s *spaceSuite) TestGetByName(c *gc.C) {
+	c.Assert(s.spaces.GetByName("space1"), gc.NotNil)
+	c.Assert(s.spaces.GetByName("space666"), gc.IsNil)
+}
 
-	c.Assert(spaces.GetByName("space1"), gc.NotNil)
-	c.Assert(spaces.GetByName("space666"), gc.IsNil)
+func (s *spaceSuite) TestGetByID(c *gc.C) {
+	c.Assert(s.spaces.GetByID("1"), gc.NotNil)
+	c.Assert(s.spaces.GetByID("999"), gc.IsNil)
 }
 
-func (*spaceSuite) TestGetByID(c *gc.C) {
-	spaces := network.SpaceInfos{
-		{ID: "space1"},
-		{ID: "space2"},
-		{ID: "space3"},
+func (s *spaceSuite) TestContainsName(c *gc.C) {
+	c.Assert(s.spaces.ContainsName("space3"), jc.IsTrue)
+	c.Assert(s.spaces.ContainsName("space666"), jc.IsFalse)
+}
+
+func (s *spaceSuite) TestMinus(c *gc.C) {
+	infos := network.SpaceInfos{
+		{ID: "2", Name: "space2"},
+		{ID: "3", Name: "space3"},
 	}
+	result := s.spaces.Minus(infos)
+	c.Assert(result, gc.DeepEquals, network.SpaceInfos{{ID: "1", Name: "space1"}})
+}
 
-	c.Assert(spaces.GetByID("space1"), gc.NotNil)
-	c.Assert(spaces.GetByID("space666"), gc.IsNil)
+func (s *spaceSuite) TestMinuxNoDiff(c *gc.C) {
+	infos := network.SpaceInfos{
+		{ID: "1", Name: "space1"},
+		{ID: "2", Name: "space2"},
+		{ID: "3", Name: "space3"},
+	}
+	result := s.spaces.Minus(infos)
+	c.Assert(result, gc.DeepEquals, network.SpaceInfos{})
 }