Merge pull request #521 from weaveworks/improve-testing

Improve testing, fix flaky deletion error

.github/PULL_REQUEST_TEMPLATE.md

@@ -1,9 +1,12 @@
 ### Description
-Please explain the changes you made here.
+
+<!-- Please explain the changes you made here. -->
 
 ### Checklist
+<!-- Delete any items if not applicable, e.g. if your name is already in `humans.txt` or doc updates are not needed. -->
 - [ ] Code compiles correctly (i.e `make build`)
 - [ ] Added tests that cover your change (if possible)
-- [ ] All tests passing (i.e. `make test`)
-- [ ] Added/modified documentation as required (such as the README)
+- [ ] All unit tests passing (i.e. `make test`)
+- [ ] All integration tests passing (i.e. `make integration-test`)
+- [ ] Added/modified documentation as required (such as the `README.md`, and `examples` directory)
 - [ ] Added yourself to the `humans.txt` file

.gitignore

@@ -4,8 +4,9 @@
 # Release config file backups
 .goreleaser.brew.combined.yml
 
-# Locally built binary
+# Locally built binaries
 /eksctl
+/eksctl-integration-test
 
 # Bad deps
 vendor/k8s.io/kops/docs

.gometalinter.json

@@ -3,13 +3,11 @@
         "vet",
         "golint",
         "errcheck",
-        "deadcode"
+        "deadcode",
+        "misspell"
     ],
     "Exclude": [
-        "^vendor\/",
-        "^build\/",
         "^pkg\/nodebootstrap\/assets.go",
-        "\/usr\/local\/go",
         "^pkg\/testutils\/ginkgo.go"
     ],
     "Deadline": "5m"

Dockerfile

@@ -36,9 +36,12 @@ RUN mkdir -p "${JUNIT_REPORT_DIR}"
 
 WORKDIR $EKSCTL
 RUN make $TEST_TARGET
-RUN make lint
 RUN make build \
     && cp ./eksctl /out/usr/local/bin/eksctl
+RUN make build-integration-test \
+    && mkdir -p /out/usr/local/share/eksctl \
+    && cp integration/*.yaml /out/usr/local/share/eksctl \
+    && cp ./eksctl-integration-test /out/usr/local/bin/eksctl-integration-test
 
 RUN go build ./vendor/github.com/kubernetes-sigs/aws-iam-authenticator/cmd/aws-iam-authenticator \
     && cp ./aws-iam-authenticator /out/usr/local/bin/aws-iam-authenticator

Makefile

@@ -8,14 +8,6 @@ EKSCTL_IMAGE ?= weaveworks/eksctl:latest
 
 .DEFAULT_GOAL := help
 
-ifneq ($(TEST_V),)
-TEST_ARGS ?= -v -ginkgo.v
-endif
-
-ifneq ($(DELETE_TEST_CLUSTER),)
-INTEGRATION_TEST_ARGS ?= -eksctl.delete=false
-endif
-
 ##@ Dependencies
 
 .PHONY: install-build-deps
@@ -30,34 +22,58 @@ build: ## Build eksctl
 
 ##@ Testing & CI
 
+ifneq ($(TEST_V),)
+UNIT_TEST_ARGS ?= -v -ginkgo.v
+INTEGRATION_TEST_ARGS ?= -test.v -ginkgo.v
+endif
+
+LINTER ?= gometalinter ./pkg/... ./cmd/... ./integration/...
+.PHONY: lint
+lint: ## Run linter over the codebase
+	@$(GOPATH)/bin/$(LINTER)
+
 .PHONY: test
 test: generate ## Run unit test (and re-generate code under test)
+	@$(MAKE) lint
 	@git diff --exit-code pkg/nodebootstrap/assets.go > /dev/null || (git --no-pager diff pkg/nodebootstrap/assets.go; exit 1)
 	@git diff --exit-code ./pkg/eks/mocks > /dev/null || (git --no-pager diff ./pkg/eks/mocks; exit 1)
 	@$(MAKE) unit-test
 	@test -z $(COVERALLS_TOKEN) || $(GOPATH)/bin/goveralls -coverprofile=coverage.out -service=circle-ci
-	@go test -tags integration ./integration/... -c && rm -f integration.test
+	@$(MAKE) build-integration-test
 
 .PHONY: unit-test
 unit-test: ## Run unit test only
-	@CGO_ENABLED=0 go test -covermode=count -coverprofile=coverage.out ./pkg/... ./cmd/... $(TEST_ARGS)
+	@CGO_ENABLED=0 go test -covermode=count -coverprofile=coverage.out ./pkg/... ./cmd/... $(UNIT_TEST_ARGS)
 
-LINTER ?= gometalinter ./...
-.PHONY: lint
-lint: ## Run linter over the codebase
-	@$(GOPATH)/bin/$(LINTER)
+.PHONY: build-integration-test
+build-integration-test: ## Build integration test binary
+	@go test -tags integration ./integration/... -c -o ./eksctl-integration-test
 
-.PHONY: ci
-ci: test lint ## Target for CI system to invoke to run tests and linting
+.PHONY: integration-test
+integration-test: build build-integration-test ## Run the integration tests (with cluster creation and cleanup)
+	@./eksctl-integration-test -test.timeout 60m \
+		$(INTEGRATION_TEST_ARGS)
+
+.PHONY: integration-test-container
+integration-test-container: eksctl-image ## Run the integration tests inside a Docker container
+	@docker run \
+	  --env=AWS_PROFILE \
+	  --volume=$(HOME)/.aws:/root/.aws \
+	  --workdir=/usr/local/share/eksctl \
+	    $(EKSCTL_IMAGE) \
+		  eksctl-integration-test \
+		    -eksctl.path=/usr/local/bin/eksctl \
+			-eksctl.kubeconfig=/tmp/kubeconfig \
+			  $(INTEGRATION_TEST_ARGS)
 
 TEST_CLUSTER ?= integration-test-dev
 .PHONY: integration-test-dev
-integration-test-dev: build ## Run the integration tests without cluster teardown. For use when developing integration tests.
+integration-test-dev: build build-integration-test ## Run the integration tests without cluster teardown. For use when developing integration tests.
 	@./eksctl utils write-kubeconfig \
 		--auto-kubeconfig \
 		--name=$(TEST_CLUSTER)
-	@go test -tags integration -timeout 21m ./integration/... \
-		$(TEST_ARGS) \
+	@./eksctl-integration-test -test.timeout 21m \
+		$(INTEGRATION_TEST_ARGS) \
 		-args \
 		-eksctl.cluster=$(TEST_CLUSTER) \
 		-eksctl.create=false \
@@ -70,10 +86,6 @@ create-integration-test-dev-cluster: build ## Create a test cluster for use when
 delete-integration-test-dev-cluster: build ## Delete the test cluster for use when developing integration tests
 	@./eksctl delete cluster --name=integration-test-dev --auto-kubeconfig
 
-.PHONY: integration-test
-integration-test: build ## Run the integration tests (with cluster creation and cleanup)
-	@go test -tags integration -timeout 60m ./integration/... $(TEST_ARGS) $(INTEGRATION_TEST_ARGS)
-
 ##@ Code Generation
 
 .PHONY: generate

pkg/apis/eksctl.io/v1alpha4/validation.go

@@ -30,8 +30,8 @@ func validateNodeGroupIAM(i int, ng *NodeGroup, value, fieldName, path string) e
 }
 
 // ValidateNodeGroupLabels uses proper Kubernetes label validation,
-// it's desinged to make sure users don't pass weird labels to the
-// nodes, which would prevent kubelets to startup propertly
+// it's designed to make sure users don't pass weird labels to the
+// nodes, which would prevent kubelets to startup properly
 func ValidateNodeGroupLabels(ng *NodeGroup) error {
 	// compact version based on:
 	// - https://github.com/kubernetes/kubernetes/blob/v1.13.2/cmd/kubelet/app/options/options.go#L257-L267

pkg/cfn/builder/api_test.go

@@ -486,7 +486,7 @@ var _ = Describe("CloudFormation template builder API", func() {
 			}))
 		})
 
-		It("should have auto-dicovery tags", func() {
+		It("should have auto-discovery tags", func() {
 			expectedTags := []Tag{
 				{
 					Key:               "Name",

pkg/cfn/builder/iam.go

@@ -152,7 +152,7 @@ func (n *NodeGroupResourceSet) addResourcesForIAM() {
 	// if neither role nor profile are given - create both
 
 	if n.spec.IAM.InstanceRoleName == "" {
-		// setting role name requires addtional capabilities
+		// setting role name requires additional capabilities
 		n.rs.withNamedIAM = true
 	}
 

pkg/cfn/manager/api.go

@@ -211,7 +211,17 @@ func (c *StackCollection) DeleteStack(name string) (*Stack, error) {
 	i := &Stack{StackName: &name}
 	s, err := c.describeStack(i)
 	if err != nil {
-		return nil, errors.Wrapf(err, "not able to get stack %q for deletion", name)
+		err = errors.Wrapf(err, "not able to get stack %q for deletion", name)
+		stacks, newErr := c.ListStacks(fmt.Sprintf("^%s$", name), cloudformation.StackStatusDeleteComplete)
+		if newErr != nil {
+			logger.Critical("not able double-check if stack was already deleted: %s", newErr.Error())
+		}
+		if count := len(stacks); count > 0 {
+			logger.Debug("%d deleted stacks found {%v}", count, stacks)
+			logger.Info("stack %q was already deleted", name)
+			return nil, nil
+		}
+		return nil, err
 	}
 	i.StackId = s.StackId
 	for _, tag := range s.Tags {

pkg/cfn/outputs/api.go

@@ -41,14 +41,14 @@ type (
 	// Collector is a callback function that takes an output value
 	// and may return an error
 	Collector func(string) error
-	// Collectors are a map of ouput names to Collector callbacks
+	// Collectors are a map of output names to Collector callbacks
 	collectors = map[string]Collector
 	// CollectorSet is a wrapper to define methods for collectors
 	CollectorSet struct{ set collectors }
 )
 
 // NewCollectorSet creates a new CollectorSet based on a map of
-// ouput names to Collector callbacks
+// output names to Collector callbacks
 func NewCollectorSet(set map[string]Collector) *CollectorSet {
 	if set == nil {
 		return &CollectorSet{make(collectors)}
@@ -67,7 +67,7 @@ func (c *CollectorSet) doCollect(must bool, stack cfn.Stack) error {
 		}
 		if value == nil {
 			if must {
-				err := fmt.Errorf("no ouput %q", key)
+				err := fmt.Errorf("no output %q", key)
 				if stack.StackName != nil {
 					return fmt.Errorf("%s in stack %q", err.Error(), *stack.StackName)
 				}

pkg/cfn/outputs/api_test.go

@@ -42,7 +42,7 @@ var _ = Describe("CloudFormation stack outputs API", func() {
 			}
 			err := Collect(stack, requiredCollectors, nil)
 			Expect(err).Should(HaveOccurred())
-			Expect(err.Error()).To(Equal("no ouput \"" + ClusterVPC + "\""))
+			Expect(err.Error()).To(Equal("no output \"" + ClusterVPC + "\""))
 		}
 
 		{
@@ -65,7 +65,7 @@ var _ = Describe("CloudFormation stack outputs API", func() {
 
 				err := Collect(stack, requiredCollectors, nil)
 				Expect(err).Should(HaveOccurred())
-				Expect(err.Error()).To(Equal("no ouput \"" + ClusterSecurityGroup + "\" in stack \"foo\""))
+				Expect(err.Error()).To(Equal("no output \"" + ClusterSecurityGroup + "\" in stack \"foo\""))
 			}
 
 			appendOutput(&stack, ClusterSecurityGroup, "sg-123")
@@ -109,7 +109,7 @@ var _ = Describe("CloudFormation stack outputs API", func() {
 			}
 			err := Collect(stack, requiredCollectors, nil)
 			Expect(err).Should(HaveOccurred())
-			Expect(err.Error()).To(Equal("no ouput \"" + ClusterVPC + "\""))
+			Expect(err.Error()).To(Equal("no output \"" + ClusterVPC + "\""))
 		}
 
 		{

pkg/ctl/cmdutils/nodegroup.go

@@ -45,7 +45,7 @@ func AddCommonCreateNodeGroupFlags(cmd *cobra.Command, fs *pflag.FlagSet, p *api
 	fs.StringVar(&ng.SSHPublicKeyPath, "ssh-public-key", defaultSSHPublicKey, "SSH public key to use for nodes (import from local path, or use existing EC2 key pair)")
 
 	fs.StringVar(&ng.AMI, "node-ami", ami.ResolverStatic, "Advanced use cases only. If 'static' is supplied (default) then eksctl will use static AMIs; if 'auto' is supplied then eksctl will automatically set the AMI based on version/region/instance type; if any other value is supplied it will override the AMI to use for the nodes. Use with extreme care.")
-	fs.StringVar(&ng.AMIFamily, "node-ami-family", ami.ImageFamilyAmazonLinux2, "Advanced use cases only. If 'AmazonLinux2' is supplied (default), then eksctl will use the offical AWS EKS AMIs (Amazon Linux 2); if 'Ubuntu1804' is supplied, then eksctl will use the offical Canonical EKS AMIs (Ubuntu 18.04).")
+	fs.StringVar(&ng.AMIFamily, "node-ami-family", ami.ImageFamilyAmazonLinux2, "Advanced use cases only. If 'AmazonLinux2' is supplied (default), then eksctl will use the official AWS EKS AMIs (Amazon Linux 2); if 'Ubuntu1804' is supplied, then eksctl will use the official Canonical EKS AMIs (Ubuntu 18.04).")
 
 	fs.BoolVarP(&ng.PrivateNetworking, "node-private-networking", "P", false, "whether to make nodegroup networking private")
 
@@ -60,9 +60,9 @@ func AddCommonCreateNodeGroupIAMAddonsFlags(fs *pflag.FlagSet, ng *api.NodeGroup
 	fs.StringSliceVar(&ng.IAM.AttachPolicyARNs, "temp-node-role-policies", []string{}, "Advanced use cases only. "+
 		"All the IAM policies to be associated to the node's instance role. "+
 		"Beware that you MUST include the policies for EKS and CNI related AWS API Access, like `arn:aws:iam::aws:policy/AmazonEKSWorkerNodePolicy` and `arn:aws:iam::aws:policy/AmazonEKS_CNI_Policy` that are used by default when this flag is omitted.")
-	fs.MarkHidden("temp-node-role-policies")
+	_ = fs.MarkHidden("temp-node-role-policies")
 	fs.StringVar(&ng.IAM.InstanceRoleName, "temp-node-role-name", "", "Advanced use cases only. Specify the exact name of the node's instance role for easier integration with K8S-IAM integrations like kube2iam. See https://github.com/weaveworks/eksctl/issues/398 for more information.")
-	fs.MarkHidden("temp-node-role-name")
+	_ = fs.MarkHidden("temp-node-role-name")
 
 	ng.IAM.WithAddonPolicies.AutoScaler = new(bool)
 	ng.IAM.WithAddonPolicies.ExternalDNS = new(bool)

pkg/ctl/completion/completion.go

@@ -21,8 +21,8 @@ To configure your bash shell to load completions for each session add to your ba
 # ~/.bashrc or ~/.profile
 . <(eksctl completion bash)
 `,
-		Run: func(cmd *cobra.Command, args []string) {
-			rootCmd.GenBashCompletion(os.Stdout)
+		RunE: func(cmd *cobra.Command, args []string) error {
+			return rootCmd.GenBashCompletion(os.Stdout)
 		},
 	}
 	var zshCompletionCmd = &cobra.Command{
@@ -38,8 +38,8 @@ and put the following in ~/.zshrc:
 fpath=($fpath ~/.zsh/completion)
 
 `,
-		Run: func(cmd *cobra.Command, args []string) {
-			rootCmd.GenZshCompletion(os.Stdout)
+		RunE: func(cmd *cobra.Command, args []string) error {
+			return rootCmd.GenZshCompletion(os.Stdout)
 		},
 	}
 

pkg/eks/nodegroup.go

@@ -45,7 +45,9 @@ func (c *ClusterProvider) CreateOrUpdateNodeGroupAuthConfigMap(clientSet *client
 		return nil
 	}
 
-	utils.UpdateAuthConfigMap(cm, ng.IAM.InstanceRoleARN)
+	if err := utils.UpdateAuthConfigMap(cm, ng.IAM.InstanceRoleARN); err != nil {
+		return errors.Wrap(err, "creating an update for auth ConfigMap")
+	}
 	if _, err := client.Update(cm); err != nil {
 		return errors.Wrap(err, "updating auth ConfigMap")
 	}