Skip to content

Avoid nodeGroups[0].name must be set validation error#1186

Merged
errordeveloper merged 7 commits intomasterfrom
fix-1164
Aug 22, 2019
Merged

Avoid nodeGroups[0].name must be set validation error#1186
errordeveloper merged 7 commits intomasterfrom
fix-1164

Conversation

@errordeveloper
Copy link
Contributor

@errordeveloper errordeveloper commented Aug 22, 2019

Description

This fixes #1164 and related cases of the same error. This is a regression that was introduced by #1116 and #1124.

Checklist

  • Code compiles correctly (i.e make build)
  • Added tests that cover your change (if possible)
  • All unit tests passing (i.e. make test)
  • Manually tested

@errordeveloper
Copy link
Contributor Author

I'm, going to have a go at adding some unit test to cover this, possibly also integration tests.

Some commands that don't have `--config-file` have custom defaulting
and validation logic. This brings the time that logic is called
before general validation and defaulting is called (i.e. `cmd.NewCtl`).
Namely, it ensures that `eksctl get ng`, `eksctl scale ng` and
`eksctl utils write-kubeconfig` work correctly.
marccarre
marccarre previously approved these changes Aug 22, 2019
@errordeveloper
Copy link
Contributor Author

errordeveloper commented Aug 22, 2019

@cPu1 what do you make of the most recent commit?

cPu1
cPu1 previously approved these changes Aug 22, 2019
@cPu1
Copy link
Contributor

cPu1 commented Aug 22, 2019

@cPu1 what do you make of the most recent commit?

LGTM.

The unit test is failing because the change to NewCreateClusterLoader isn't updated in configfile_test.go:126

@errordeveloper
Copy link
Contributor Author

Yes, I'll fix the unit tests and try to add some new ones also....

@errordeveloper
Copy link
Contributor Author

I'd like to add eksctl get ng to integration suite as well.

- validate remote points at upstream
- push to release branch instead of master
})

It("loader should handle nodegroup exclusion with config file", func() {
loaderParams := []struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternative way:

		loaderParams := []struct {
			configFile     string
			nodeGroupCount int
		}{
			{"01-simple-cluster.yaml", 1, true},
			{"02-custom-vpc-cidr-no-nodes.yaml", 0, true},
			{"03-two-nodegroups.yaml", 2, true},
			{"05-advanced-nodegroups.yaml", 3, true},
			{"07-ssh-keys.yaml", 5, true},
		}
		
		for _, loaderTest := range loaderParams {
			cmd := &Cmd{
				CobraCommand:      newCmd(),
				ClusterConfigFile: filepath.Join(examplesDir, loaderTest.configFile),
				ClusterConfig:     api.NewClusterConfig(),
				ProviderConfig:    &api.ProviderConfig{},
			}
		
			ngFilter := NewNodeGroupFilter()
		
			for _, withoutNodeGroup := range []bool{true, false} {
				Expect(NewCreateClusterLoader(cmd, ngFilter, nil, withoutNodeGroup).Load()).To(Succeed())
		
				Expect(ngFilter.ExcludeAll).To(Equal(withoutNodeGroup))
		
				Expect(cmd.ClusterConfig.NodeGroups).To(HaveLen(loaderTest.nodeGroupCount))
		
				_, err := cmd.NewCtl()
				Expect(err).ToNot(HaveOccurred())
		
			}
		}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I thought about it, but didn't seem to make that much difference

@errordeveloper errordeveloper merged commit 4b654fe into master Aug 22, 2019
@errordeveloper errordeveloper deleted the fix-1164 branch August 22, 2019 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

get nodegroups command no longer display all nodegroups

3 participants