Merge pull request #665 from weaveworks/cluster-config-loader

Create `ClusterConfigLoader` abstraction

pkg/ctl/cmdutils/cmdutils.go

@@ -89,3 +89,13 @@ func ErrUnsupportedRegion(p *api.ProviderConfig) error {
 func ErrNameFlagAndArg(nameFlag, nameArg string) error {
 	return fmt.Errorf("--name=%s and argument %s %s", nameFlag, nameArg, IncompatibleFlags)
 }
+
+// ErrMustBeSet is a common error message
+func ErrMustBeSet(pathOrFlag string) error {
+	return fmt.Errorf("%s must be set", pathOrFlag)
+}
+
+// ErrCannotUseWithConfigFile is a common error message
+func ErrCannotUseWithConfigFile(what string) error {
+	return fmt.Errorf("cannot use %s when --config-file/-f is set", what)
+}

pkg/ctl/cmdutils/cmdutils_suite_test.go

@@ -0,0 +1,11 @@
+package cmdutils_test
+
+import (
+	"testing"
+
+	"github.com/weaveworks/eksctl/pkg/testutils"
+)
+
+func TestSuite(t *testing.T) {
+	testutils.RegisterAndRun(t)
+}

pkg/ctl/cmdutils/configfile.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
+	"k8s.io/apimachinery/pkg/util/sets"
 
 	api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha4"
 	"github.com/weaveworks/eksctl/pkg/eks"
@@ -15,63 +16,106 @@ func AddConfigFileFlag(path *string, fs *pflag.FlagSet) {
 	fs.StringVarP(path, "config-file", "f", "", "load configuration from a file")
 }
 
-// LoadMetadata handles loading of clusterConfigFile vs using flags for all commands that require only
-// metadata fileds, e.g. `eksctl delete cluster` or `eksctl utils update-kube-proxy` and other similar
-// commands that do simple operations against existing clusters
-func LoadMetadata(p *api.ProviderConfig, cfg *api.ClusterConfig, clusterConfigFile, nameArg string, cmd *cobra.Command) error {
-	meta := cfg.Metadata
+// ClusterConfigLoader holds common parameters required for loading
+// ClusterConfig objects from files vs using flags fallback
+type ClusterConfigLoader struct {
+	provider *api.ProviderConfig
+	path     string
+	cmd      *cobra.Command
 
-	if clusterConfigFile != "" {
-		if err := eks.LoadConfigFromFile(clusterConfigFile, cfg); err != nil {
-			return err
-		}
-		meta = cfg.Metadata
+	Spec    *api.ClusterConfig
+	NameArg string
 
-		incompatibleFlags := []string{
-			"name",
-			"region",
-			"version",
-		}
+	FlagsIncompatibleWithConfigFile, FlagsIncompatibleWithoutConfigFile sets.String
+
+	ValidateWithConfigFile, ValidateWithoutConfigFile func() error
+}
+
+// NewClusterConfigLoader constructs a standard loader
+func NewClusterConfigLoader(provider *api.ProviderConfig, spec *api.ClusterConfig, clusterConfigFile string, cmd *cobra.Command) *ClusterConfigLoader {
+	nilValidator := func() error { return nil }
+
+	return &ClusterConfigLoader{
+		provider: provider,
+		path:     clusterConfigFile,
+		cmd:      cmd,
 
-		for _, f := range incompatibleFlags {
-			if flag := cmd.Flag(f); flag != nil && flag.Changed {
-				return fmt.Errorf("cannot use --%s when --config-file/-f is set", f)
+		Spec: spec,
+
+		FlagsIncompatibleWithConfigFile:    sets.NewString("name", "region", "version"),
+		ValidateWithConfigFile:             nilValidator,
+		FlagsIncompatibleWithoutConfigFile: sets.NewString(),
+		ValidateWithoutConfigFile:          nilValidator,
+	}
+}
+
+// Load ClusterConfig or use flags
+func (l *ClusterConfigLoader) Load() error {
+	if err := api.Register(); err != nil {
+		return err
+	}
+
+	if l.path == "" {
+		for f := range l.FlagsIncompatibleWithoutConfigFile {
+			if l.cmd.Flag(f).Changed {
+				return fmt.Errorf("cannot use --%s unless a config file is specified via --config-file/-f", f)
 			}
 		}
+		return l.ValidateWithoutConfigFile()
+	}
 
-		if nameArg != "" {
-			return fmt.Errorf("cannot use name argument %q when --config-file/-f is set", nameArg)
-		}
+	if err := eks.LoadConfigFromFile(l.path, l.Spec); err != nil {
+		return err
+	}
+	meta := l.Spec.Metadata
 
-		if meta.Name == "" {
-			return fmt.Errorf("metadata.name must be set")
+	for f := range l.FlagsIncompatibleWithConfigFile {
+		if flag := l.cmd.Flag(f); flag != nil && flag.Changed {
+			return ErrCannotUseWithConfigFile(fmt.Sprintf("--%s", f))
 		}
+	}
 
-		// region is always required when config file is used
-		if meta.Region == "" {
-			return fmt.Errorf("metadata.region must be set")
-		}
+	if l.NameArg != "" {
+		return ErrCannotUseWithConfigFile(fmt.Sprintf("name argument %q", l.NameArg))
+	}
+
+	if meta.Name == "" {
+		return ErrMustBeSet("metadata.name")
+	}
+
+	if meta.Region == "" {
+		return ErrMustBeSet("metadata.region")
+	}
+	l.provider.Region = meta.Region
+
+	return l.ValidateWithConfigFile()
+}
+
+// LoadMetadata handles loading of clusterConfigFile vs using flags for all commands that require only
+// metadata fileds, e.g. `eksctl delete cluster` or `eksctl utils update-kube-proxy` and other similar
+// commands that do simple operations against existing clusters
+func LoadMetadata(provider *api.ProviderConfig, spec *api.ClusterConfig, clusterConfigFile, nameArg string, cmd *cobra.Command) error {
+	l := NewClusterConfigLoader(provider, spec, clusterConfigFile, cmd)
+
+	l.NameArg = nameArg
 
-		p.Region = meta.Region
+	l.ValidateWithoutConfigFile = func() error {
+		meta := l.Spec.Metadata
 
-		// version has different default values in some command
-		// we don't check it here
-	} else {
-		if meta.Name != "" && nameArg != "" {
-			return ErrNameFlagAndArg(meta.Name, nameArg)
+		if meta.Name != "" && l.NameArg != "" {
+			return ErrNameFlagAndArg(meta.Name, l.NameArg)
 		}
 
-		if nameArg != "" {
-			meta.Name = nameArg
+		if l.NameArg != "" {
+			meta.Name = l.NameArg
 		}
 
 		if meta.Name == "" {
-			return fmt.Errorf("--name must be set")
+			return ErrMustBeSet("--name")
 		}
 
-		// default region will get picked by eks.New, and
-		// version validation gets handled separately
+		return nil
 	}
 
-	return nil
+	return l.Load()
 }

pkg/ctl/cmdutils/configfile_test.go

@@ -0,0 +1,75 @@
+package cmdutils_test
+
+import (
+	"path/filepath"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	"github.com/spf13/cobra"
+
+	api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha4"
+	. "github.com/weaveworks/eksctl/pkg/ctl/cmdutils"
+	// "github.com/weaveworks/eksctl/pkg/printers"
+)
+
+var _ = Describe("cmdutils configfile", func() {
+
+	newCmd := func() *cobra.Command {
+		return &cobra.Command{
+			Use: "test",
+			Run: func(_ *cobra.Command, _ []string) {},
+		}
+	}
+
+	const examplesDir = "../../../examples/"
+
+	Context("load configfiles", func() {
+
+		It("should handle name argument", func() {
+			cfg := api.NewClusterConfig()
+
+			err := LoadMetadata(nil, cfg, "", "foo-1", nil)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(cfg.Metadata.Name).To(Equal("foo-1"))
+
+			err = LoadMetadata(nil, cfg, "", "foo-2", nil)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(Equal("--name=foo-1 and argument foo-2 cannot be used at the same time"))
+
+			cmd := newCmd()
+
+			err = LoadMetadata(nil, cfg, examplesDir+"01-simple-cluster.yaml", "foo-3", cmd)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(Equal(ErrCannotUseWithConfigFile(`name argument "foo-3"`).Error()))
+
+			fs := cmd.Flags()
+
+			fs.StringVar(&cfg.Metadata.Name, "name", "", "")
+			cmd.Flag("name").Changed = true
+
+			Expect(cmd.Flag("name").Changed).To(BeTrue())
+			err = LoadMetadata(nil, cfg, examplesDir+"01-simple-cluster.yaml", "foo-3", cmd)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(Equal(ErrCannotUseWithConfigFile("--name").Error()))
+		})
+
+		It("load all of example file", func() {
+			examples, err := filepath.Glob(examplesDir + "*.yaml")
+			Expect(err).ToNot(HaveOccurred())
+
+			Expect(examples).To(HaveLen(5))
+			for _, example := range examples {
+				cfg := api.NewClusterConfig()
+
+				p := &api.ProviderConfig{}
+				err := LoadMetadata(p, cfg, example, "", newCmd())
+
+				Expect(err).ToNot(HaveOccurred())
+				Expect(cfg.Metadata.Name).ToNot(BeEmpty())
+				Expect(cfg.Metadata.Region).ToNot(BeEmpty())
+				Expect(cfg.Metadata.Region).To(Equal(p.Region))
+				Expect(cfg.Metadata.Version).To(BeEmpty())
+			}
+		})
+	})
+})

pkg/ctl/delete/cluster.go

@@ -55,10 +55,6 @@ func deleteClusterCmd(g *cmdutils.Grouping) *cobra.Command {
 func doDeleteCluster(p *api.ProviderConfig, cfg *api.ClusterConfig, nameArg string, cmd *cobra.Command) error {
 	printer := printers.NewJSONPrinter()
 
-	if err := api.Register(); err != nil {
-		return err
-	}
-
 	if err := cmdutils.LoadMetadata(p, cfg, clusterConfigFile, nameArg, cmd); err != nil {
 		return err
 	}

pkg/ctl/update/cluster.go

@@ -57,10 +57,6 @@ func updateClusterCmd(g *cmdutils.Grouping) *cobra.Command {
 }
 
 func doUpdateClusterCmd(p *api.ProviderConfig, cfg *api.ClusterConfig, nameArg string, cmd *cobra.Command) error {
-	if err := api.Register(); err != nil {
-		return err
-	}
-
 	if err := cmdutils.LoadMetadata(p, cfg, clusterConfigFile, nameArg, cmd); err != nil {
 		return err
 	}

pkg/ctl/utils/install_corends.go

@@ -46,10 +46,6 @@ func installCoreDNSCmd(g *cmdutils.Grouping) *cobra.Command {
 }
 
 func doInstallCoreDNS(p *api.ProviderConfig, cfg *api.ClusterConfig, nameArg string, cmd *cobra.Command) error {
-	if err := api.Register(); err != nil {
-		return err
-	}
-
 	if err := cmdutils.LoadMetadata(p, cfg, clusterConfigFile, nameArg, cmd); err != nil {
 		return err
 	}

pkg/ctl/utils/update_aws_node.go

@@ -45,10 +45,6 @@ func updateAWSNodeCmd(g *cmdutils.Grouping) *cobra.Command {
 }
 
 func doUpdateAWSNode(p *api.ProviderConfig, cfg *api.ClusterConfig, nameArg string, cmd *cobra.Command) error {
-	if err := api.Register(); err != nil {
-		return err
-	}
-
 	if err := cmdutils.LoadMetadata(p, cfg, clusterConfigFile, nameArg, cmd); err != nil {
 		return err
 	}

pkg/ctl/utils/update_kube_proxy.go

@@ -45,10 +45,6 @@ func updateKubeProxyCmd(g *cmdutils.Grouping) *cobra.Command {
 }
 
 func doUpdateKubeProxy(p *api.ProviderConfig, cfg *api.ClusterConfig, nameArg string, cmd *cobra.Command) error {
-	if err := api.Register(); err != nil {
-		return err
-	}
-
 	if err := cmdutils.LoadMetadata(p, cfg, clusterConfigFile, nameArg, cmd); err != nil {
 		return err
 	}