Skip to content

Commit

Permalink
Refactor command functions
Browse files Browse the repository at this point in the history 
- create short-hand methods for defining standard commands
- move most of common parameters to a struct
- get rid of global state
- improve overall consistency of command functions
  • Loading branch information
@errordeveloper
errordeveloper committed Jun 7, 2019
1 parent 5af41e0 commit a82c616
Show file tree
Hide file tree
Showing 33 changed files with 719 additions and 940 deletions.
30 changes: 22 additions & 8 deletions pkg/ctl/cmdutils/cmdutils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,20 @@ import (
// IncompatibleFlags is a common substring of an error message
const IncompatibleFlags = "cannot be used at the same time"

// NewVerbCmd defines a standard verb command
func NewVerbCmd(use, short, long string) *cobra.Command {
return &cobra.Command{
Use: use,
Short: short,
Long: long,
Run: func(c *cobra.Command, _ []string) {
if err := c.Help(); err != nil {
logger.Debug("ignoring error %q", err.Error())
}
},
}
}

// AddPreRun chains cmd.PreRun handlers, as cobra only allows one, so we don't
// accidentially override one we registered earlier
func AddPreRun(cmd *cobra.Command, newFn func(cmd *cobra.Command, args []string)) {
Expand Down Expand Up @@ -53,11 +67,11 @@ func LogPlanModeWarning(plan bool) {
}

// AddApproveFlag adds common `--approve` flag
func AddApproveFlag(plan *bool, cmd *cobra.Command, fs *pflag.FlagSet) {
approve := fs.Bool("approve", !*plan, "Apply the changes")
AddPreRun(cmd, func(cmd *cobra.Command, args []string) {
func AddApproveFlag(fs *pflag.FlagSet, rc *ResourceCmd) {
approve := fs.Bool("approve", !rc.Plan, "Apply the changes")
AddPreRun(rc.Command, func(cmd *cobra.Command, args []string) {
if cmd.Flag("approve").Changed {
*plan = !*approve
rc.Plan = !*approve
}
})
}
Expand Down Expand Up @@ -111,12 +125,12 @@ func AddVersionFlag(fs *pflag.FlagSet, meta *api.ClusterMeta, extraUsageInfo str
}

// AddWaitFlag adds common --wait flag
func AddWaitFlag(wait *bool, fs *pflag.FlagSet, description string) {
func AddWaitFlag(fs *pflag.FlagSet, wait *bool, description string) {
fs.BoolVarP(wait, "wait", "w", *wait, fmt.Sprintf("wait for %s before exiting", description))
}

// AddUpdateAuthConfigMap adds common --update-auth-configmap flag
func AddUpdateAuthConfigMap(updateAuthConfigMap *bool, fs *pflag.FlagSet, description string) {
func AddUpdateAuthConfigMap(fs *pflag.FlagSet, updateAuthConfigMap *bool, description string) {
fs.BoolVar(updateAuthConfigMap, "update-auth-configmap", true, description)
}

Expand All @@ -134,8 +148,8 @@ func AddCommonFlagsForGetCmd(fs *pflag.FlagSet, chunkSize *int, outputMode *stri
}

// ErrUnsupportedRegion is a common error message
func ErrUnsupportedRegion(p *api.ProviderConfig) error {
return fmt.Errorf("--region=%s is not supported - use one of: %s", p.Region, strings.Join(api.SupportedRegions(), ", "))
func ErrUnsupportedRegion(provider *api.ProviderConfig) error {
return fmt.Errorf("--region=%s is not supported - use one of: %s", provider.Region, strings.Join(api.SupportedRegions(), ", "))
}

// ErrNameFlagAndArg is a common error message
Expand Down
114 changes: 49 additions & 65 deletions pkg/ctl/cmdutils/configfile.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package cmdutils
import (
"fmt"

"github.com/spf13/cobra"
"github.com/spf13/pflag"
"k8s.io/apimachinery/pkg/util/sets"

Expand All @@ -12,7 +11,7 @@ import (
)

// AddConfigFileFlag adds common --config-file flag
func AddConfigFileFlag(path *string, fs *pflag.FlagSet) {
func AddConfigFileFlag(fs *pflag.FlagSet, path *string) {
fs.StringVarP(path, "config-file", "f", "", "load configuration from a file")
}

Expand All @@ -22,25 +21,18 @@ type ClusterConfigLoader interface {
}

type commonClusterConfigLoader struct {
provider *api.ProviderConfig
path string
cmd *cobra.Command
spec *api.ClusterConfig
nameArg string
*ResourceCmd

flagsIncompatibleWithConfigFile, flagsIncompatibleWithoutConfigFile sets.String

validateWithConfigFile, validateWithoutConfigFile func() error
}

func newCommonClusterConfigLoader(provider *api.ProviderConfig, spec *api.ClusterConfig, clusterConfigFile string, cmd *cobra.Command) *commonClusterConfigLoader {
func newCommonClusterConfigLoader(rc *ResourceCmd) *commonClusterConfigLoader {
nilValidatorFunc := func() error { return nil }

return &commonClusterConfigLoader{
provider: provider,
path: clusterConfigFile,
cmd: cmd,
spec: spec,
ResourceCmd: rc,

validateWithConfigFile: nilValidatorFunc,
flagsIncompatibleWithConfigFile: sets.NewString("name", "region", "version"),
Expand All @@ -55,28 +47,28 @@ func (l *commonClusterConfigLoader) Load() error {
return err
}

if l.path == "" {
if l.ClusterConfigFile == "" {
for f := range l.flagsIncompatibleWithoutConfigFile {
if flag := l.cmd.Flag(f); flag != nil && flag.Changed {
if flag := l.Command.Flag(f); flag != nil && flag.Changed {
return fmt.Errorf("cannot use --%s unless a config file is specified via --config-file/-f", f)
}
}
return l.validateWithoutConfigFile()
}

if err := eks.LoadConfigFromFile(l.path, l.spec); err != nil {
if err := eks.LoadConfigFromFile(l.ClusterConfigFile, l.ClusterConfig); err != nil {
return err
}
meta := l.spec.Metadata
meta := l.ClusterConfig.Metadata

for f := range l.flagsIncompatibleWithConfigFile {
if flag := l.cmd.Flag(f); flag != nil && flag.Changed {
if flag := l.Command.Flag(f); flag != nil && flag.Changed {
return ErrCannotUseWithConfigFile(fmt.Sprintf("--%s", f))
}
}

if l.nameArg != "" {
return ErrCannotUseWithConfigFile(fmt.Sprintf("name argument %q", l.nameArg))
if l.NameArg != "" {
return ErrCannotUseWithConfigFile(fmt.Sprintf("name argument %q", l.NameArg))
}

if meta.Name == "" {
Expand All @@ -86,28 +78,26 @@ func (l *commonClusterConfigLoader) Load() error {
if meta.Region == "" {
return ErrMustBeSet("metadata.region")
}
l.provider.Region = meta.Region
l.ProviderConfig.Region = meta.Region

return l.validateWithConfigFile()
}

// NewMetadataLoader 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 NewMetadataLoader(provider *api.ProviderConfig, spec *api.ClusterConfig, clusterConfigFile, nameArg string, cmd *cobra.Command) ClusterConfigLoader {
l := newCommonClusterConfigLoader(provider, spec, clusterConfigFile, cmd)

l.nameArg = nameArg
func NewMetadataLoader(rc *ResourceCmd) ClusterConfigLoader {
l := newCommonClusterConfigLoader(rc)

l.validateWithoutConfigFile = func() error {
meta := l.spec.Metadata
meta := l.ClusterConfig.Metadata

if meta.Name != "" && l.nameArg != "" {
return ErrNameFlagAndArg(meta.Name, l.nameArg)
if meta.Name != "" && l.NameArg != "" {
return ErrNameFlagAndArg(meta.Name, l.NameArg)
}

if l.nameArg != "" {
meta.Name = l.nameArg
if l.NameArg != "" {
meta.Name = l.NameArg
}

if meta.Name == "" {
Expand All @@ -121,10 +111,8 @@ func NewMetadataLoader(provider *api.ProviderConfig, spec *api.ClusterConfig, cl
}

// NewCreateClusterLoader will laod config or use flags for 'eksctl create cluster'
func NewCreateClusterLoader(provider *api.ProviderConfig, spec *api.ClusterConfig, clusterConfigFile, nameArg string, cmd *cobra.Command, ngFilter *NodeGroupFilter) ClusterConfigLoader {
l := newCommonClusterConfigLoader(provider, spec, clusterConfigFile, cmd)

l.nameArg = nameArg
func NewCreateClusterLoader(rc *ResourceCmd, ngFilter *NodeGroupFilter) ClusterConfigLoader {
l := newCommonClusterConfigLoader(rc)

l.flagsIncompatibleWithConfigFile.Insert(
"tags",
Expand Down Expand Up @@ -155,32 +143,32 @@ func NewCreateClusterLoader(provider *api.ProviderConfig, spec *api.ClusterConfi
)

l.validateWithConfigFile = func() error {
if l.spec.VPC == nil {
l.spec.VPC = api.NewClusterVPC()
if l.ClusterConfig.VPC == nil {
l.ClusterConfig.VPC = api.NewClusterVPC()
}

if l.spec.HasAnySubnets() && len(l.spec.AvailabilityZones) != 0 {
if l.ClusterConfig.HasAnySubnets() && len(l.ClusterConfig.AvailabilityZones) != 0 {
return fmt.Errorf("vpc.subnets and availabilityZones cannot be set at the same time")
}

return nil
}

l.validateWithoutConfigFile = func() error {
meta := l.spec.Metadata
meta := l.ClusterConfig.Metadata

// generate cluster name or use either flag or argument
if ClusterName(meta.Name, l.nameArg) == "" {
return ErrNameFlagAndArg(meta.Name, l.nameArg)
if ClusterName(meta.Name, l.NameArg) == "" {
return ErrNameFlagAndArg(meta.Name, l.NameArg)
}
meta.Name = ClusterName(meta.Name, l.nameArg)
meta.Name = ClusterName(meta.Name, l.NameArg)

if l.spec.Status != nil {
if l.ClusterConfig.Status != nil {
return fmt.Errorf("status fields are read-only")
}

return ngFilter.ForEach(l.spec.NodeGroups, func(i int, ng *api.NodeGroup) error {
if cmd.Flag("ssh-public-key").Changed {
return ngFilter.ForEach(l.ClusterConfig.NodeGroups, func(i int, ng *api.NodeGroup) error {
if l.Command.Flag("ssh-public-key").Changed {
if *ng.SSH.PublicKeyPath == "" {
return fmt.Errorf("--ssh-public-key must be non-empty string")
}
Expand All @@ -200,10 +188,8 @@ func NewCreateClusterLoader(provider *api.ProviderConfig, spec *api.ClusterConfi
}

// NewCreateNodeGroupLoader will laod config or use flags for 'eksctl create nodegroup'
func NewCreateNodeGroupLoader(provider *api.ProviderConfig, spec *api.ClusterConfig, clusterConfigFile, nameArg string, cmd *cobra.Command, ngFilter *NodeGroupFilter, include, exclude []string) ClusterConfigLoader {
l := newCommonClusterConfigLoader(provider, spec, clusterConfigFile, cmd)

l.nameArg = nameArg
func NewCreateNodeGroupLoader(rc *ResourceCmd, ngFilter *NodeGroupFilter) ClusterConfigLoader {
l := newCommonClusterConfigLoader(rc)

l.flagsIncompatibleWithConfigFile.Insert(
"cluster",
Expand All @@ -228,7 +214,7 @@ func NewCreateNodeGroupLoader(provider *api.ProviderConfig, spec *api.ClusterCon
)

l.validateWithConfigFile = func() error {
if err := ngFilter.AppendGlobs(include, exclude, spec.NodeGroups); err != nil {
if err := ngFilter.AppendGlobs(l.IncludeNodeGroups, l.ExcludeNodeGroups, l.ClusterConfig.NodeGroups); err != nil {
return err
}
return nil
Expand All @@ -241,13 +227,13 @@ func NewCreateNodeGroupLoader(provider *api.ProviderConfig, spec *api.ClusterCon
)

l.validateWithoutConfigFile = func() error {
if spec.Metadata.Name == "" {
if l.ClusterConfig.Metadata.Name == "" {
return ErrMustBeSet("--cluster")
}

return ngFilter.ForEach(spec.NodeGroups, func(i int, ng *api.NodeGroup) error {
return ngFilter.ForEach(l.ClusterConfig.NodeGroups, func(i int, ng *api.NodeGroup) error {

if cmd.Flag("ssh-public-key").Changed {
if l.Command.Flag("ssh-public-key").Changed {
if *ng.SSH.PublicKeyPath == "" {
return fmt.Errorf("--ssh-public-key must be non-empty string")
}
Expand All @@ -257,10 +243,10 @@ func NewCreateNodeGroupLoader(provider *api.ProviderConfig, spec *api.ClusterCon
}

// generate nodegroup name or use either flag or argument
if NodeGroupName(ng.Name, l.nameArg) == "" {
return ErrNameFlagAndArg(ng.Name, l.nameArg)
if NodeGroupName(ng.Name, l.NameArg) == "" {
return ErrNameFlagAndArg(ng.Name, l.NameArg)
}
ng.Name = NodeGroupName(ng.Name, l.nameArg)
ng.Name = NodeGroupName(ng.Name, l.NameArg)

return nil
})
Expand All @@ -270,17 +256,15 @@ func NewCreateNodeGroupLoader(provider *api.ProviderConfig, spec *api.ClusterCon
}

// NewDeleteNodeGroupLoader will laod config or use flags for 'eksctl delete nodegroup'
func NewDeleteNodeGroupLoader(provider *api.ProviderConfig, spec *api.ClusterConfig, ng *api.NodeGroup, clusterConfigFile, nameArg string, cmd *cobra.Command, ngFilter *NodeGroupFilter, include, exclude []string, plan *bool) ClusterConfigLoader {
l := newCommonClusterConfigLoader(provider, spec, clusterConfigFile, cmd)

l.nameArg = nameArg
func NewDeleteNodeGroupLoader(rc *ResourceCmd, ng *api.NodeGroup, ngFilter *NodeGroupFilter) ClusterConfigLoader {
l := newCommonClusterConfigLoader(rc)

l.flagsIncompatibleWithConfigFile.Insert(
"cluster",
)

l.validateWithConfigFile = func() error {
return ngFilter.AppendGlobs(include, exclude, spec.NodeGroups)
return ngFilter.AppendGlobs(l.IncludeNodeGroups, l.ExcludeNodeGroups, l.ClusterConfig.NodeGroups)
}

l.flagsIncompatibleWithoutConfigFile.Insert(
Expand All @@ -292,16 +276,16 @@ func NewDeleteNodeGroupLoader(provider *api.ProviderConfig, spec *api.ClusterCon
)

l.validateWithoutConfigFile = func() error {
if l.spec.Metadata.Name == "" {
if l.ClusterConfig.Metadata.Name == "" {
return ErrMustBeSet("--cluster")
}

if ng.Name != "" && nameArg != "" {
return ErrNameFlagAndArg(ng.Name, nameArg)
if ng.Name != "" && l.NameArg != "" {
return ErrNameFlagAndArg(ng.Name, l.NameArg)
}

if nameArg != "" {
ng.Name = nameArg
if l.NameArg != "" {
ng.Name = l.NameArg
}

if ng.Name == "" {
Expand All @@ -310,7 +294,7 @@ func NewDeleteNodeGroupLoader(provider *api.ProviderConfig, spec *api.ClusterCon

ngFilter.AppendIncludeNames(ng.Name)

*plan = false
l.Plan = false

return nil
}
Expand Down
Loading

0 comments on commit a82c616

Please sign in to comment.