Skip to content

Improve consistency of command functions #1124

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
errordeveloper merged 1 commit into from
Aug 9, 2019
Merged

Improve consistency of command functions #1124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/release_notes/0.4.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
- ...

## Bug Fixes and code improvements
- perform defaulting and validation in all commands (#1124)
- improve nodegroup validation to detect duplicate names (#1116)
- tidy up command usage formatting (#1112)
- various internal improvements (#1113, #1117, #1118, #1119)
- various internal improvements (#1113, #1117, #1118, #1119)
4 changes: 1 addition & 3 deletions pkg/apis/eksctl.io/v1alpha5/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ func SetClusterConfigDefaults(cfg *ClusterConfig) {
}

// SetNodeGroupDefaults will set defaults for a given nodegroup
func SetNodeGroupDefaults(_ int, ng *NodeGroup) error {
func SetNodeGroupDefaults(_ int, ng *NodeGroup) {
if ng.InstanceType == "" {
if HasMixedInstances(ng) {
ng.InstanceType = "mixed"
Expand Down Expand Up @@ -96,8 +96,6 @@ func SetNodeGroupDefaults(_ int, ng *NodeGroup) error {
if ng.IAM.WithAddonPolicies.EFS == nil {
ng.IAM.WithAddonPolicies.EFS = Disabled()
}

return nil
}

// DefaultClusterNAT will set the default value for Cluster NAT mode
Expand Down
14 changes: 0 additions & 14 deletions pkg/ctl/cmdutils/nodegroup_filter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,20 +112,6 @@ func (f *NodeGroupFilter) ForEach(nodeGroups []*api.NodeGroup, iterFn func(i int
return nil
}

// ValidateNodeGroupsAndSetDefaults is calls api.ValidateNodeGroup & api.SetNodeGroupDefaults for
// all nodegroups that match the filter
func (f *NodeGroupFilter) ValidateNodeGroupsAndSetDefaults(nodeGroups []*api.NodeGroup) error {
return f.ForEach(nodeGroups, func(i int, ng *api.NodeGroup) error {
if err := api.ValidateNodeGroup(i, ng); err != nil {
return err
}
if err := api.SetNodeGroupDefaults(i, ng); err != nil {
return err
}
return nil
})
}

func (*NodeGroupFilter) collectNames(nodeGroups []*api.NodeGroup) []string {
names := []string{}
for _, ng := range nodeGroups {
Expand Down
17 changes: 14 additions & 3 deletions pkg/ctl/cmdutils/nodegroup_filter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ var _ = Describe("nodegroup filter", func() {

Context("ForEach", func() {

It("should iterate over unique nodegroups and apply defaults with ValidateNodeGroupsAndSetDefaults", func() {
It("should iterate over unique nodegroups, apply defaults and validate", func() {
cfg := newClusterConfig()
addGroupA(cfg)
addGroupB(cfg)
Expand All @@ -138,7 +138,12 @@ var _ = Describe("nodegroup filter", func() {
printer := printers.NewJSONPrinter()
names := []string{}

filter.ValidateNodeGroupsAndSetDefaults(cfg.NodeGroups)
filter.ForEach(cfg.NodeGroups, func(i int, nodeGroup *api.NodeGroup) error {
api.SetNodeGroupDefaults(i, nodeGroup)
err := api.ValidateNodeGroup(i, nodeGroup)
Expect(err).ToNot(HaveOccurred())
return nil
})

filter.ForEach(cfg.NodeGroups, func(i int, nodeGroup *api.NodeGroup) error {
Expect(nodeGroup).To(Equal(cfg.NodeGroups[i]))
Expand All @@ -161,7 +166,13 @@ var _ = Describe("nodegroup filter", func() {

filter := NewNodeGroupFilter()
filter.ExcludeAll = true
filter.ValidateNodeGroupsAndSetDefaults(cfg.NodeGroups)

filter.ForEach(cfg.NodeGroups, func(i int, nodeGroup *api.NodeGroup) error {
api.SetNodeGroupDefaults(i, nodeGroup)
err := api.ValidateNodeGroup(i, nodeGroup)
Expect(err).ToNot(HaveOccurred())
return nil
})

callback := false
filter.ForEach(cfg.NodeGroups, func(_ int, _ *api.NodeGroup) error {
Expand Down
42 changes: 39 additions & 3 deletions pkg/ctl/cmdutils/resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,15 @@ import (
"github.com/spf13/cobra"

api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5"
"github.com/weaveworks/eksctl/pkg/eks"
)

// ResourceCmd holds attributes that most of the commands use
type ResourceCmd struct {
Command *cobra.Command
FlagSetGroup *NamedFlagSetGroup

Plan, Wait bool
Plan, Wait, Validate bool

NameArg string

Expand All @@ -26,14 +27,49 @@ type ResourceCmd struct {
Include, Exclude []string
}

// NewCtl performs common defaulting and validation and constructs a new
// instance of eks.ClusterProvider, it may return an error if configuration
// is invalid or region is not supported
func (rc *ResourceCmd) NewCtl() (*eks.ClusterProvider, error) {
api.SetClusterConfigDefaults(rc.ClusterConfig)

if err := api.ValidateClusterConfig(rc.ClusterConfig); err != nil {
if rc.Validate {
return nil, err
}
logger.Warning("ignoring validation error: %s", err.Error())
}

for i, ng := range rc.ClusterConfig.NodeGroups {
if err := api.ValidateNodeGroup(i, ng); err != nil {
if rc.Validate {
return nil, err
}
logger.Warning("ignoring validation error: %s", err.Error())
}
// defaulting of nodegroup currently depends on validation;
// that may change, but at present that's how it's meant to work
api.SetNodeGroupDefaults(i, ng)
}

ctl := eks.New(rc.ProviderConfig, rc.ClusterConfig)
Copy link
Contributor Author

@errordeveloper errordeveloper Aug 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be something like:

if rc.MockProvider != nil {
  return rc.MockProvider, nil
}

ctl := eks.New(rc.ProviderConfig, rc.ClusterConfig)

Copy link
Contributor Author

@errordeveloper errordeveloper Aug 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But let's leave until next iteration, I also have a PR to rename rc to simply cmd, which I promised to Martina a long time ago.


if !ctl.IsSupportedRegion() {
return nil, ErrUnsupportedRegion(rc.ProviderConfig)
}

return ctl, nil
}

// AddResourceCmd create a registers a new command under the given verb command
func AddResourceCmd(flagGrouping *FlagGrouping, parentVerbCmd *cobra.Command, newResourceCmd func(*ResourceCmd)) {
resource := &ResourceCmd{
Command: &cobra.Command{},
ProviderConfig: &api.ProviderConfig{},

Plan: true, // always on by default
Wait: false, // varies in some commands
Plan: true, // always on by default
Wait: false, // varies in some commands
Validate: true, // also on by default
}
resource.FlagSetGroup = flagGrouping.New(resource.Command)
newResourceCmd(resource)
Expand Down
19 changes: 4 additions & 15 deletions pkg/ctl/create/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5"
"github.com/weaveworks/eksctl/pkg/authconfigmap"
"github.com/weaveworks/eksctl/pkg/ctl/cmdutils"
"github.com/weaveworks/eksctl/pkg/eks"
"github.com/weaveworks/eksctl/pkg/kops"
"github.com/weaveworks/eksctl/pkg/printers"
"github.com/weaveworks/eksctl/pkg/utils"
Expand Down Expand Up @@ -97,21 +96,11 @@ func doCreateCluster(rc *cmdutils.ResourceCmd, params *createClusterCmdParams) e
cfg := rc.ClusterConfig
meta := rc.ClusterConfig.Metadata

api.SetClusterConfigDefaults(cfg)

if err := api.ValidateClusterConfig(cfg); err != nil {
return err
}

if err := ngFilter.ValidateNodeGroupsAndSetDefaults(cfg.NodeGroups); err != nil {
return err
}

printer := printers.NewJSONPrinter()
ctl := eks.New(rc.ProviderConfig, cfg)

if !ctl.IsSupportedRegion() {
return cmdutils.ErrUnsupportedRegion(rc.ProviderConfig)
ctl, err := rc.NewCtl()
Copy link
Contributor

@cPu1 cPu1 Aug 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we pass a ClusterProvider as an argument to command handlers like doCreateCluster, instead of constructing it here, we can inject a ClusterProvider with fake AWS API clients for api.ClusterProvider. That'd let us unit-test command handlers.

Copy link
Contributor Author

@errordeveloper errordeveloper Aug 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, but let's make that change separately. I'm just trying to take care of consistency here. I think that change will be easier to make now.

Actually, we could just make it happen inside of NewCtl, i.e. make it possible to set the provider implementation in ResourceCmd (which gets constructed before the handler function runs).

Copy link
Contributor

@cPu1 cPu1 Aug 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to do it later in a separate PR.

if err != nil {
return err
}
logger.Info("using region %s", meta.Region)

Expand Down Expand Up @@ -241,7 +230,7 @@ func doCreateCluster(rc *cmdutils.ResourceCmd, params *createClusterCmdParams) e
return err
}

err := ngFilter.ForEach(cfg.NodeGroups, func(_ int, ng *api.NodeGroup) error {
err = ngFilter.ForEach(cfg.NodeGroups, func(_ int, ng *api.NodeGroup) error {
// resolve AMI
if err := ctl.EnsureAMI(meta.Version, ng); err != nil {
return err
Expand Down
7 changes: 5 additions & 2 deletions pkg/ctl/create/iamidentitymapping.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5"
"github.com/weaveworks/eksctl/pkg/authconfigmap"
"github.com/weaveworks/eksctl/pkg/ctl/cmdutils"
"github.com/weaveworks/eksctl/pkg/eks"
)

func createIAMIdentityMappingCmd(rc *cmdutils.ResourceCmd) {
Expand Down Expand Up @@ -50,7 +49,11 @@ func doCreateIAMIdentityMapping(rc *cmdutils.ResourceCmd, id *authconfigmap.MapR

cfg := rc.ClusterConfig

ctl := eks.New(rc.ProviderConfig, cfg)
ctl, err := rc.NewCtl()
if err != nil {
return err
}
logger.Info("using region %s", cfg.Metadata.Region)

if err := ctl.CheckAuth(); err != nil {
return err
Expand Down
19 changes: 4 additions & 15 deletions pkg/ctl/create/nodegroup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5"
"github.com/weaveworks/eksctl/pkg/authconfigmap"
"github.com/weaveworks/eksctl/pkg/ctl/cmdutils"
"github.com/weaveworks/eksctl/pkg/eks"
"github.com/weaveworks/eksctl/pkg/printers"
"github.com/weaveworks/eksctl/pkg/utils"
)
Expand Down Expand Up @@ -64,21 +63,11 @@ func doCreateNodeGroups(rc *cmdutils.ResourceCmd, updateAuthConfigMap bool) erro
cfg := rc.ClusterConfig
meta := rc.ClusterConfig.Metadata

api.SetClusterConfigDefaults(cfg)

if err := api.ValidateClusterConfig(cfg); err != nil {
return err
}

if err := ngFilter.ValidateNodeGroupsAndSetDefaults(cfg.NodeGroups); err != nil {
return err
}

printer := printers.NewJSONPrinter()
ctl := eks.New(rc.ProviderConfig, cfg)

if !ctl.IsSupportedRegion() {
return cmdutils.ErrUnsupportedRegion(rc.ProviderConfig)
ctl, err := rc.NewCtl()
if err != nil {
return err
}
logger.Info("using region %s", meta.Region)

Expand All @@ -104,7 +93,7 @@ func doCreateNodeGroups(rc *cmdutils.ResourceCmd, updateAuthConfigMap bool) erro
return err
}

err := ngFilter.ForEach(cfg.NodeGroups, func(_ int, ng *api.NodeGroup) error {
err = ngFilter.ForEach(cfg.NodeGroups, func(_ int, ng *api.NodeGroup) error {
// resolve AMI
if err := ctl.EnsureAMI(meta.Version, ng); err != nil {
return err
Expand Down
7 changes: 3 additions & 4 deletions pkg/ctl/delete/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5"
"github.com/weaveworks/eksctl/pkg/cfn/manager"
"github.com/weaveworks/eksctl/pkg/ctl/cmdutils"
"github.com/weaveworks/eksctl/pkg/eks"
"github.com/weaveworks/eksctl/pkg/elb"
"github.com/weaveworks/eksctl/pkg/printers"
"github.com/weaveworks/eksctl/pkg/ssh"
Expand Down Expand Up @@ -77,10 +76,10 @@ func doDeleteCluster(rc *cmdutils.ResourceCmd) error {
meta := rc.ClusterConfig.Metadata

printer := printers.NewJSONPrinter()
ctl := eks.New(rc.ProviderConfig, cfg)

if !ctl.IsSupportedRegion() {
return cmdutils.ErrUnsupportedRegion(rc.ProviderConfig)
ctl, err := rc.NewCtl()
if err != nil {
return err
}
logger.Info("using region %s", meta.Region)

Expand Down
7 changes: 5 additions & 2 deletions pkg/ctl/delete/iamidentitymapping.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5"
"github.com/weaveworks/eksctl/pkg/authconfigmap"
"github.com/weaveworks/eksctl/pkg/ctl/cmdutils"
"github.com/weaveworks/eksctl/pkg/eks"
)

func deleteIAMIdentityMappingCmd(rc *cmdutils.ResourceCmd) {
Expand Down Expand Up @@ -44,7 +43,11 @@ func doDeleteIAMIdentityMapping(rc *cmdutils.ResourceCmd, role string, all bool)

cfg := rc.ClusterConfig

ctl := eks.New(rc.ProviderConfig, cfg)
ctl, err := rc.NewCtl()
if err != nil {
return err
}
logger.Info("using region %s", cfg.Metadata.Region)

if err := ctl.CheckAuth(); err != nil {
return err
Expand Down
7 changes: 5 additions & 2 deletions pkg/ctl/delete/nodegroup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"github.com/weaveworks/eksctl/pkg/authconfigmap"
"github.com/weaveworks/eksctl/pkg/ctl/cmdutils"
"github.com/weaveworks/eksctl/pkg/drain"
"github.com/weaveworks/eksctl/pkg/eks"
)

func deleteNodeGroupCmd(rc *cmdutils.ResourceCmd) {
Expand Down Expand Up @@ -53,7 +52,11 @@ func doDeleteNodeGroup(rc *cmdutils.ResourceCmd, ng *api.NodeGroup, updateAuthCo

cfg := rc.ClusterConfig

ctl := eks.New(rc.ProviderConfig, cfg)
ctl, err := rc.NewCtl()
if err != nil {
return err
}
logger.Info("using region %s", cfg.Metadata.Region)

if err := ctl.CheckAuth(); err != nil {
return err
Expand Down
7 changes: 5 additions & 2 deletions pkg/ctl/drain/nodegroup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (

api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5"
"github.com/weaveworks/eksctl/pkg/ctl/cmdutils"
"github.com/weaveworks/eksctl/pkg/eks"

"github.com/weaveworks/eksctl/pkg/drain"
)
Expand Down Expand Up @@ -49,7 +48,11 @@ func doDrainNodeGroup(rc *cmdutils.ResourceCmd, ng *api.NodeGroup, undo, onlyMis

cfg := rc.ClusterConfig

ctl := eks.New(rc.ProviderConfig, cfg)
ctl, err := rc.NewCtl()
if err != nil {
return err
}
logger.Info("using region %s", cfg.Metadata.Region)

if err := ctl.CheckAuth(); err != nil {
return err
Expand Down
8 changes: 3 additions & 5 deletions pkg/ctl/get/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"github.com/spf13/pflag"
api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5"
"github.com/weaveworks/eksctl/pkg/ctl/cmdutils"
"github.com/weaveworks/eksctl/pkg/eks"
)

func getClusterCmd(rc *cmdutils.ResourceCmd) {
Expand Down Expand Up @@ -39,10 +38,9 @@ func doGetCluster(rc *cmdutils.ResourceCmd, params *getCmdParams, listAllRegions
cfg := rc.ClusterConfig
regionGiven := cfg.Metadata.Region != "" // eks.New resets this field, so we need to check if it was set in the fist place

ctl := eks.New(rc.ProviderConfig, cfg)

if !ctl.IsSupportedRegion() {
return cmdutils.ErrUnsupportedRegion(rc.ProviderConfig)
ctl, err := rc.NewCtl()
if err != nil {
return err
}

if regionGiven && listAllRegions {
Expand Down
6 changes: 4 additions & 2 deletions pkg/ctl/get/iamidentitymapping.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
api "github.com/weaveworks/eksctl/pkg/apis/eksctl.io/v1alpha5"
"github.com/weaveworks/eksctl/pkg/authconfigmap"
"github.com/weaveworks/eksctl/pkg/ctl/cmdutils"
"github.com/weaveworks/eksctl/pkg/eks"
"github.com/weaveworks/eksctl/pkg/printers"
)

Expand Down Expand Up @@ -47,7 +46,10 @@ func doGetIAMIdentityMapping(rc *cmdutils.ResourceCmd, params *getCmdParams, rol

cfg := rc.ClusterConfig

ctl := eks.New(rc.ProviderConfig, cfg)
ctl, err := rc.NewCtl()
if err != nil {
return err
}

if err := ctl.CheckAuth(); err != nil {
return err
Expand Down
Loading