Code review fixes, introduce storage attrs type

jangocheng · Oct 9, 2020 · e655b23 · e655b23
1 parent 74f38fa
commit e655b23
Show file tree

Hide file tree

Showing 14 changed files with 44 additions and 38 deletions.
diff --git a/agent/agentbootstrap/bootstrap_test.go b/agent/agentbootstrap/bootstrap_test.go
@@ -186,7 +186,7 @@ LXC_BRIDGE="ignored"`[1:])
 			ModelConstraints:              expectModelConstraints,
 			ControllerInheritedConfig:     controllerInheritedConfig,
 			HostedModelConfig:             hostedModelConfigAttrs,
-			StoragePools: map[string]map[string]interface{}{
+			StoragePools: map[string]storage.Attrs{
 				"spool": {
 					"type": "loop",
 					"foo":  "bar",

diff --git a/caas/kubernetes/provider/storage_test.go b/caas/kubernetes/provider/storage_test.go
@@ -41,7 +41,7 @@ func (s *storageSuite) TestValidateConfig(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 	err = p.ValidateConfig(cfg)
 	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(cfg.Attrs(), jc.DeepEquals, map[string]interface{}{
+	c.Assert(cfg.Attrs(), jc.DeepEquals, storage.Attrs{
 		"storage-class":       "my-storage",
 		"storage-provisioner": "aws-storage",
 		"storage-label":       "storage-fred",

diff --git a/cloudconfig/instancecfg/instancecfg.go b/cloudconfig/instancecfg/instancecfg.go
@@ -38,6 +38,7 @@ import (
 	"github.com/juju/juju/mongo"
 	"github.com/juju/juju/service"
 	"github.com/juju/juju/service/common"
+	"github.com/juju/juju/storage"
 	coretools "github.com/juju/juju/tools"
 )
 
@@ -342,7 +343,7 @@ type StateInitializationParams struct {
 
 	// StoragePools is one or more named storage pools to create
 	// in the controller model.
-	StoragePools map[string]map[string]interface{}
+	StoragePools map[string]storage.Attrs
 }
 
 type stateInitializationParamsInternal struct {
@@ -352,7 +353,7 @@ type stateInitializationParamsInternal struct {
 	ControllerInheritedConfig               map[string]interface{}            `yaml:"controller-config-defaults,omitempty"`
 	RegionInheritedConfig                   cloud.RegionConfig                `yaml:"region-inherited-config,omitempty"`
 	HostedModelConfig                       map[string]interface{}            `yaml:"hosted-model-config,omitempty"`
-	StoragePools                            map[string]map[string]interface{} `yaml:"storage-pools,omitempty"`
+	StoragePools                            map[string]storage.Attrs          `yaml:"storage-pools,omitempty"`
 	BootstrapMachineInstanceId              instance.Id                       `yaml:"bootstrap-machine-instance-id,omitempty"`
 	BootstrapMachineConstraints             constraints.Value                 `yaml:"bootstrap-machine-constraints"`
 	BootstrapMachineHardwareCharacteristics *instance.HardwareCharacteristics `yaml:"bootstrap-machine-hardware,omitempty"`

diff --git a/cmd/juju/commands/bootstrap.go b/cmd/juju/commands/bootstrap.go
@@ -1250,7 +1250,7 @@ type bootstrapConfigs struct {
 	bootstrap                bootstrap.Config
 	inheritedControllerAttrs map[string]interface{}
 	userConfigAttrs          map[string]interface{}
-	storagePools             map[string]map[string]interface{}
+	storagePools             map[string]storage.Attrs
 }
 
 func (c *bootstrapCommand) bootstrapConfigs(
@@ -1314,7 +1314,7 @@ func (c *bootstrapCommand) bootstrapConfigs(
 	if err != nil {
 		return bootstrapConfigs{}, errors.Trace(err)
 	}
-	var storagePools map[string]map[string]interface{}
+	var storagePools map[string]storage.Attrs
 	if len(storagePoolAttrs) > 0 {
 		poolName, _ := storagePoolAttrs[poolmanager.Name].(string)
 		if poolName == "" {
@@ -1324,7 +1324,7 @@ func (c *bootstrapCommand) bootstrapConfigs(
 		if poolType == "" {
 			return bootstrapConfigs{}, errors.NewNotValid(nil, "storage pool requires a type")
 		}
-		storagePools = make(map[string]map[string]interface{})
+		storagePools = make(map[string]storage.Attrs)
 		storagePools[poolName] = storagePoolAttrs
 	}
 

diff --git a/cmd/juju/commands/bootstrap_test.go b/cmd/juju/commands/bootstrap_test.go
@@ -59,6 +59,7 @@ import (
 	"github.com/juju/juju/provider/dummy"
 	_ "github.com/juju/juju/provider/ec2"
 	"github.com/juju/juju/provider/openstack"
+	"github.com/juju/juju/storage"
 	coretesting "github.com/juju/juju/testing"
 	coretools "github.com/juju/juju/tools"
 	jujuversion "github.com/juju/juju/version"
@@ -951,7 +952,7 @@ func (s *BootstrapSuite) TestBootstrapWithStoragePool(c *gc.C) {
 		"--storage-pool", "foo=bar",
 	)
 
-	c.Assert(bootstrapFuncs.args.StoragePools, jc.DeepEquals, map[string]map[string]interface{}{
+	c.Assert(bootstrapFuncs.args.StoragePools, jc.DeepEquals, map[string]storage.Attrs{
 		"test": {
 			"name": "test",
 			"type": "loop",

diff --git a/environs/bootstrap.go b/environs/bootstrap.go
@@ -14,6 +14,7 @@ import (
 	"github.com/juju/juju/controller"
 	"github.com/juju/juju/core/constraints"
 	"github.com/juju/juju/environs/imagemetadata"
+	"github.com/juju/juju/storage"
 	"github.com/juju/juju/tools"
 )
 
@@ -43,7 +44,7 @@ type BootstrapParams struct {
 
 	// StoragePools is one or more named storage pools to create
 	// in the controller model.
-	StoragePools map[string]map[string]interface{}
+	StoragePools map[string]storage.Attrs
 
 	// BootstrapSeries, if specified, is the series to use for the
 	// initial bootstrap machine.

diff --git a/environs/bootstrap/bootstrap.go b/environs/bootstrap/bootstrap.go
@@ -40,6 +40,7 @@ import (
 	"github.com/juju/juju/environs/tools"
 	"github.com/juju/juju/mongo"
 	"github.com/juju/juju/pki"
+	corestorage "github.com/juju/juju/storage"
 	coretools "github.com/juju/juju/tools"
 	jujuversion "github.com/juju/juju/version"
 )
@@ -168,7 +169,7 @@ type BootstrapParams struct {
 
 	// StoragePools is one or more named storage pools to create
 	// in the controller model.
-	StoragePools map[string]map[string]interface{}
+	StoragePools map[string]corestorage.Attrs
 
 	// Force is used to allow a bootstrap to be run on unsupported series.
 	Force bool

diff --git a/environs/bootstrap/bootstrap_test.go b/environs/bootstrap/bootstrap_test.go
@@ -47,6 +47,7 @@ import (
 	"github.com/juju/juju/juju/keys"
 	"github.com/juju/juju/mongo"
 	"github.com/juju/juju/provider/dummy"
+	corestorage "github.com/juju/juju/storage"
 	coretesting "github.com/juju/juju/testing"
 	"github.com/juju/juju/tools"
 	jujuversion "github.com/juju/juju/version"
@@ -203,7 +204,7 @@ func (s *bootstrapSuite) TestBootstrapWithStoragePools(c *gc.C) {
 			AdminSecret:              "admin-secret",
 			CAPrivateKey:             coretesting.CAKey,
 			SupportedBootstrapSeries: supportedJujuSeries,
-			StoragePools: map[string]map[string]interface{}{
+			StoragePools: map[string]corestorage.Attrs{
 				"spool": {
 					"type": "loop",
 					"foo":  "bar",
@@ -212,7 +213,7 @@ func (s *bootstrapSuite) TestBootstrapWithStoragePools(c *gc.C) {
 		})
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(env.bootstrapCount, gc.Equals, 1)
-	c.Assert(env.args.StoragePools, gc.DeepEquals, map[string]map[string]interface{}{
+	c.Assert(env.args.StoragePools, gc.DeepEquals, map[string]corestorage.Attrs{
 		"spool": {
 			"type": "loop",
 			"foo":  "bar",

diff --git a/provider/common/bootstrap_test.go b/provider/common/bootstrap_test.go
@@ -531,7 +531,7 @@ func (s *BootstrapSuite) TestStartInstanceRootDisk(c *gc.C) {
 		AvailableTools:           availableTools,
 		SupportedBootstrapSeries: coretesting.FakeSupportedJujuSeries,
 		BootstrapConstraints:     constraints.MustParse("root-disk-source=spool"),
-		StoragePools: map[string]map[string]interface{}{
+		StoragePools: map[string]corestorage.Attrs{
 			"spool": {
 				"type": "dummy",
 				"foo":  "bar",

diff --git a/state/initialize.go b/state/initialize.go
@@ -33,7 +33,7 @@ type InitializeParams struct {
 
 	// StoragePools is one or more named storage pools to create
 	// in the controller model.
-	StoragePools map[string]map[string]interface{}
+	StoragePools map[string]storage.Attrs
 
 	// Cloud contains the properties of the cloud that the
 	// controller runs in.
@@ -440,7 +440,7 @@ func (st *State) createDefaultStoragePoolsOps(registry storage.ProviderRegistry)
 	return ops, nil
 }
 
-func (st *State) createCustomStoragePoolsOps(registry storage.ProviderRegistry, storagePools map[string]map[string]interface{}) ([]txn.Op, error) {
+func (st *State) createCustomStoragePoolsOps(registry storage.ProviderRegistry, storagePools map[string]storage.Attrs) ([]txn.Op, error) {
 	m := poolmanager.MemSettings{make(map[string]map[string]interface{})}
 	pm := poolmanager.New(m, registry)
 	for name, attrs := range storagePools {
@@ -456,8 +456,6 @@ func (st *State) createCustomStoragePoolsOps(registry storage.ProviderRegistry,
 		}
 	}
 
-	logger.Criticalf("SP: %#v", m.Settings)
-
 	var ops []txn.Op
 	for key, settings := range m.Settings {
 		ops = append(ops, createSettingsOp(settingsC, key, settings))

diff --git a/state/initialize_test.go b/state/initialize_test.go
@@ -698,7 +698,7 @@ func (s *InitializeSuite) TestInitializeWithStoragePool(c *gc.C) {
 		},
 		MongoSession:  s.Session,
 		AdminPassword: "dummy-secret",
-		StoragePools: map[string]map[string]interface{}{
+		StoragePools: map[string]storage.Attrs{
 			"spool": {
 				"type": "dummy",
 				"foo":  "bar",

diff --git a/state/migration_import_test.go b/state/migration_import_test.go
@@ -2216,7 +2216,7 @@ func (s *MigrationImportSuite) TestStoragePools(c *gc.C) {
 	pool := pools[0]
 	c.Assert(pool.Name(), gc.Equals, "test-pool")
 	c.Assert(pool.Provider(), gc.Equals, provider.LoopProviderType)
-	c.Assert(pool.Attrs(), jc.DeepEquals, map[string]interface{}{
+	c.Assert(pool.Attrs(), jc.DeepEquals, storage.Attrs{
 		"value": 42,
 	})
 }

diff --git a/storage/config.go b/storage/config.go
@@ -21,11 +21,14 @@ const (
 	ConfigStorageDir = "storage-dir"
 )
 
+// Attrs defines storage attributes.
+type Attrs map[string]interface{}
+
 // Config defines the configuration for a storage source.
 type Config struct {
 	name     string
 	provider ProviderType
-	attrs    map[string]interface{}
+	attrs    Attrs
 }
 
 var fields = schema.Fields{}
@@ -36,7 +39,7 @@ var configChecker = schema.FieldMap(
 )
 
 // NewConfig creates a new Config for instantiating a storage source.
-func NewConfig(name string, provider ProviderType, attrs map[string]interface{}) (*Config, error) {
+func NewConfig(name string, provider ProviderType, attrs Attrs) (*Config, error) {
 	_, err := configChecker.Coerce(attrs, nil)
 	if err != nil {
 		return nil, errors.Annotate(err, "validating common storage config")
@@ -60,11 +63,11 @@ func (c *Config) Provider() ProviderType {
 }
 
 // Attrs returns the configuration attributes for a storage source.
-func (c *Config) Attrs() map[string]interface{} {
+func (c *Config) Attrs() Attrs {
 	if c.attrs == nil {
 		return nil
 	}
-	attrs := make(map[string]interface{})
+	attrs := make(Attrs)
 	for k, v := range c.attrs {
 		attrs[k] = v
 	}

diff --git a/storage/poolmanager/poolmanager_test.go b/storage/poolmanager/poolmanager_test.go
@@ -53,7 +53,7 @@ func (s *poolSuite) TestList(c *gc.C) {
 	pools, err := s.poolManager.List()
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(pools, gc.HasLen, 1)
-	c.Assert(pools[0].Attrs(), gc.DeepEquals, map[string]interface{}{"foo": "bar", "bleep": "bloop"})
+	c.Assert(pools[0].Attrs(), gc.DeepEquals, storage.Attrs{"foo": "bar", "bleep": "bloop"})
 	c.Assert(pools[0].Name(), gc.Equals, "testpool")
 	c.Assert(pools[0].Provider(), gc.Equals, storage.ProviderType("loop"))
 }
@@ -87,36 +87,36 @@ func (s *poolSuite) TestPool(c *gc.C) {
 	s.createSettings(c)
 	p, err := s.poolManager.Get("testpool")
 	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(p.Attrs(), gc.DeepEquals, map[string]interface{}{"foo": "bar", "bleep": "bloop"})
+	c.Assert(p.Attrs(), gc.DeepEquals, storage.Attrs{"foo": "bar", "bleep": "bloop"})
 	c.Assert(p.Name(), gc.Equals, "testpool")
 	c.Assert(p.Provider(), gc.Equals, storage.ProviderType("loop"))
 }
 
 func (s *poolSuite) TestCreate(c *gc.C) {
-	created, err := s.poolManager.Create("testpool", storage.ProviderType("loop"), map[string]interface{}{"foo": "bar"})
+	created, err := s.poolManager.Create("testpool", storage.ProviderType("loop"), storage.Attrs{"foo": "bar"})
 	c.Assert(err, jc.ErrorIsNil)
 	p, err := s.poolManager.Get("testpool")
 	c.Assert(created, gc.DeepEquals, p)
 	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(p.Attrs(), gc.DeepEquals, map[string]interface{}{"foo": "bar"})
+	c.Assert(p.Attrs(), gc.DeepEquals, storage.Attrs{"foo": "bar"})
 	c.Assert(p.Name(), gc.Equals, "testpool")
 	c.Assert(p.Provider(), gc.Equals, storage.ProviderType("loop"))
 }
 
 func (s *poolSuite) TestCreateAlreadyExists(c *gc.C) {
-	_, err := s.poolManager.Create("testpool", storage.ProviderType("loop"), map[string]interface{}{"foo": "bar"})
+	_, err := s.poolManager.Create("testpool", storage.ProviderType("loop"), storage.Attrs{"foo": "bar"})
 	c.Assert(err, jc.ErrorIsNil)
-	_, err = s.poolManager.Create("testpool", storage.ProviderType("loop"), map[string]interface{}{"foo": "bar"})
+	_, err = s.poolManager.Create("testpool", storage.ProviderType("loop"), storage.Attrs{"foo": "bar"})
 	c.Assert(err, gc.ErrorMatches, ".*cannot overwrite.*")
 }
 
 func (s *poolSuite) TestCreateMissingName(c *gc.C) {
-	_, err := s.poolManager.Create("", "loop", map[string]interface{}{"foo": "bar"})
+	_, err := s.poolManager.Create("", "loop", storage.Attrs{"foo": "bar"})
 	c.Assert(err, gc.ErrorMatches, "pool name is missing")
 }
 
 func (s *poolSuite) TestCreateMissingType(c *gc.C) {
-	_, err := s.poolManager.Create("testpool", "", map[string]interface{}{"foo": "bar"})
+	_, err := s.poolManager.Create("testpool", "", storage.Attrs{"foo": "bar"})
 	c.Assert(err, gc.ErrorMatches, "provider type is missing")
 }
 
@@ -132,28 +132,28 @@ func (s *poolSuite) TestCreateInvalidConfig(c *gc.C) {
 
 func (s *poolSuite) TestReplace(c *gc.C) {
 	s.createSettings(c)
-	err := s.poolManager.Replace("testpool", "", map[string]interface{}{"zip": "zap"})
+	err := s.poolManager.Replace("testpool", "", storage.Attrs{"zip": "zap"})
 	c.Assert(err, jc.ErrorIsNil)
 	p, err := s.poolManager.Get("testpool")
 	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(p.Attrs(), gc.DeepEquals, map[string]interface{}{"zip": "zap"})
+	c.Assert(p.Attrs(), gc.DeepEquals, storage.Attrs{"zip": "zap"})
 	c.Assert(p.Name(), gc.Equals, "testpool")
 	c.Assert(p.Provider(), gc.Equals, storage.ProviderType("loop"))
 }
 
 func (s *poolSuite) TestReplaceMissingName(c *gc.C) {
-	err := s.poolManager.Replace("", "", map[string]interface{}{"foo": "bar"})
+	err := s.poolManager.Replace("", "", storage.Attrs{"foo": "bar"})
 	c.Assert(err, gc.ErrorMatches, "pool name is missing")
 }
 
 func (s *poolSuite) TestReplaceNewProvider(c *gc.C) {
 	s.registry.Providers["notebook"] = &dummystorage.StorageProvider{}
 	s.createSettings(c)
-	err := s.poolManager.Replace("testpool", "notebook", map[string]interface{}{"handwritten": "true"})
+	err := s.poolManager.Replace("testpool", "notebook", storage.Attrs{"handwritten": "true"})
 	c.Assert(err, jc.ErrorIsNil)
 	p, err := s.poolManager.Get("testpool")
 	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(p.Attrs(), gc.DeepEquals, map[string]interface{}{"handwritten": "true"})
+	c.Assert(p.Attrs(), gc.DeepEquals, storage.Attrs{"handwritten": "true"})
 	c.Assert(p.Name(), gc.Equals, "testpool")
 	c.Assert(p.Provider(), gc.Equals, storage.ProviderType("notebook"))
 }
@@ -165,12 +165,12 @@ func (s *poolSuite) TestReplaceInvalidConfig(c *gc.C) {
 		},
 	}
 	s.createSettings(c)
-	err := s.poolManager.Replace("testpool", "invalid", map[string]interface{}{"zip": "zap"})
+	err := s.poolManager.Replace("testpool", "invalid", storage.Attrs{"zip": "zap"})
 	c.Assert(err, gc.ErrorMatches, "validating storage provider config: no good")
 }
 
 func (s *poolSuite) TestReplaceNotFound(c *gc.C) {
-	err := s.poolManager.Replace("deadpool", "", map[string]interface{}{"zip": "zap"})
+	err := s.poolManager.Replace("deadpool", "", storage.Attrs{"zip": "zap"})
 	c.Assert(err, gc.ErrorMatches, "pool \"deadpool\" not found")
 }