Repository: argoproj-labs/applicationset
Pull request #63 (2020-11-21 01:06:09)

Add .golangci.yml from Argo CD, and fix corresponding linter failures
I've added the `.golangci.yml` from Argo CD, with the same enabled linters that it uses, and then fixed all the failing lines. 

OmerKahani: +1
jgwest: Hi @xianlubird @dgoodwin @OmerKahani, this PR is ready to be reviewed/merged, feel free to let me know if anything else is required.
jgwest: Rebased and ready to go.

Patch:
diff --git a/.golangci.yml b/.golangci.yml
new file mode 100644
index 0000000..df6f3b9
--- /dev/null
+++ b/.golangci.yml
@@ -0,0 +1,14 @@
+run:
+  timeout: 2m
+  skip-files:
+  skip-dirs:
+linters:
+  enable:
+    - vet
+    - deadcode
+    - goimports
+    - varcheck
+    - structcheck
+    - ineffassign
+    - unconvert
+    - unparam
diff --git a/Makefile b/Makefile
index 474e981..bf0fe38 100644
--- a/Makefile
+++ b/Makefile
@@ -6,6 +6,10 @@ DOCKER_PUSH?=true
 build:
 	CGO_ENABLED=0 go build -ldflags="-w -s" -o ./dist/argocd-applicationset .
 
+.PHONY: test
+test:
+	go test -race -count=1 `go list ./...`
+
 .PHONY: image
 image:
 	docker build -t $(IMAGE) .
@@ -22,6 +26,11 @@ manifests:
 	controller-gen paths=./api/... crd:trivialVersions=true output:dir=./manifests/crds/
 	controller-gen object paths=./api/...
 
+lint:
+	golangci-lint --version
+	GOMAXPROCS=2 golangci-lint run --fix --verbose --timeout 300s
+
+
 # Run go fmt against code
 fmt:
 	go fmt ./...
diff --git a/main.go b/main.go
index 986175c..5930ae4 100644
--- a/main.go
+++ b/main.go
@@ -3,6 +3,7 @@ package main
 import (
 	"context"
 	"flag"
+
 	"github.com/argoproj-labs/applicationset/pkg/generators"
 	"github.com/argoproj-labs/applicationset/pkg/services"
 	"github.com/argoproj-labs/applicationset/pkg/utils"
diff --git a/pkg/controllers/applicationset_controller.go b/pkg/controllers/applicationset_controller.go
index e5df7f9..4079070 100644
--- a/pkg/controllers/applicationset_controller.go
+++ b/pkg/controllers/applicationset_controller.go
@@ -18,8 +18,11 @@ package controllers
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"reflect"
+	"sort"
+	"strings"
 	"time"
 
 	"github.com/argoproj-labs/applicationset/pkg/generators"
@@ -67,11 +70,29 @@ func (r *ApplicationSetReconciler) Reconcile(req ctrl.Request) (ctrl.Result, err
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
+	// Log a warning if there are unrecognized generators
+	checkInvalidGenerators(&applicationSetInfo)
+
 	// desiredApplications is the main list of all expected Applications from all generators in this appset.
 	desiredApplications, err := r.generateApplications(applicationSetInfo)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
+	if hasDuplicates, name := hasDuplicateNames(desiredApplications); hasDuplicates {
+		// The reconciler presumes that any errors that are returned are a signal
+		// that the resource should attempt to be reconciled again (causing
+		// Reconcile to be called again, which will return the same error, ad
+		// infinitum until we are exponentially backed off).
+		//
+		// In this case, since we know that what the user provided is incorrect
+		// (we successfully generated and templated their ApplicationSet, but the
+		// result of that was bad), no matter how many times we try to do so it
+		// will fail. So just log it and return that the resource was
+		// successfully reconciled (which is true... it was reconciled to an
+		// error condition).
+		log.Errorf("ApplicationSet %s contains applications with duplicate name: %s", applicationSetInfo.Name, name)
+		return ctrl.Result{}, nil
+	}
 
 	if r.Policy.Update() {
 		err = r.createOrUpdateInCluster(ctx, applicationSetInfo, desiredApplications)
@@ -100,6 +121,101 @@ func (r *ApplicationSetReconciler) Reconcile(req ctrl.Request) (ctrl.Result, err
 	}, nil
 }
 
+// Log a warning if there are unrecognized generators
+func checkInvalidGenerators(applicationSetInfo *argoprojiov1alpha1.ApplicationSet) {
+	hasInvalidGenerators, invalidGenerators := invalidGenerators(applicationSetInfo)
+	if len(invalidGenerators) > 0 {
+		gnames := []string{}
+		for n := range invalidGenerators {
+			gnames = append(gnames, n)
+		}
+		sort.Strings(gnames)
+		aname := applicationSetInfo.ObjectMeta.Name
+		msg := "ApplicationSet %s contains unrecognized generators: %s"
+		log.Warnf(msg, aname, strings.Join(gnames, ", "))
+	} else if hasInvalidGenerators {
+		name := applicationSetInfo.ObjectMeta.Name
+		msg := "ApplicationSet %s contains unrecognized generators"
+		log.Warnf(msg, name)
+	}
+}
+
+// Return true if there are unknown generators specified in the application set.  If we can discover the names
+// of these generators, return the names as the keys in a map
+func invalidGenerators(applicationSetInfo *argoprojiov1alpha1.ApplicationSet) (bool, map[string]bool) {
+	names := make(map[string]bool)
+	hasInvalidGenerators := false
+	for index, generator := range applicationSetInfo.Spec.Generators {
+		v := reflect.Indirect(reflect.ValueOf(generator))
+		found := false
+		for i := 0; i < v.NumField(); i++ {
+			field := v.Field(i)
+			if !field.CanInterface() {
+				continue
+			}
+			if !reflect.ValueOf(field.Interface()).IsNil() {
+				found = true
+				break
+			}
+		}
+		if !found {
+			hasInvalidGenerators = true
+			addInvalidGeneratorNames(names, applicationSetInfo, index)
+		}
+	}
+	return hasInvalidGenerators, names
+}
+
+func addInvalidGeneratorNames(names map[string]bool, applicationSetInfo *argoprojiov1alpha1.ApplicationSet, index int) {
+	// The generator names are stored in the "kubectl.kubernetes.io/last-applied-configuration" annotation
+	config := applicationSetInfo.ObjectMeta.Annotations["kubectl.kubernetes.io/last-applied-configuration"]
+	var values map[string]interface{}
+	err := json.Unmarshal([]byte(config), &values)
+	if err != nil {
+		log.Warnf("couldn't unmarshal kubectl.kubernetes.io/last-applied-configuration: %+v", config)
+		return
+	}
+
+	spec, ok := values["spec"].(map[string]interface{})
+	if !ok {
+		log.Warn("coundn't get spec from kubectl.kubernetes.io/last-applied-configuration annotation")
+		return
+	}
+
+	generators, ok := spec["generators"].([]interface{})
+	if !ok {
+		log.Warn("coundn't get generators from kubectl.kubernetes.io/last-applied-configuration annotation")
+		return
+	}
+
+	if index >= len(generators) {
+		log.Warnf("index %d out of range %d for generator in kubectl.kubernetes.io/last-applied-configuration", index, len(generators))
+		return
+	}
+
+	generator, ok := generators[index].(map[string]interface{})
+	if !ok {
+		log.Warn("coundn't get generator from kubectl.kubernetes.io/last-applied-configuration annotation")
+		return
+	}
+
+	for key := range generator {
+		names[key] = true
+		break
+	}
+}
+
+func hasDuplicateNames(applications []argov1alpha1.Application) (bool, string) {
+	nameSet := map[string]struct{}{}
+	for _, app := range applications {
+		if _, present := nameSet[app.Name]; present {
+			return true, app.Name
+		}
+		nameSet[app.Name] = struct{}{}
+	}
+	return false, ""
+}
+
 func (r *ApplicationSetReconciler) GetRelevantGenerators(requestedGenerator *argoprojiov1alpha1.ApplicationSetGenerator) []generators.Generator {
 	var res []generators.Generator
 
@@ -280,7 +396,8 @@ func (r *ApplicationSetReconciler) createInCluster(ctx context.Context, applicat
 	return r.createOrUpdateInCluster(ctx, applicationSet, createApps)
 }
 
-func (r *ApplicationSetReconciler) getCurrentApplications(ctx context.Context, applicationSet argoprojiov1alpha1.ApplicationSet) ([]argov1alpha1.Application, error) {
+func (r *ApplicationSetReconciler) getCurrentApplications(_ context.Context, applicationSet argoprojiov1alpha1.ApplicationSet) ([]argov1alpha1.Application, error) {
+	// TODO: Should this use the context param?
 	var current argov1alpha1.ApplicationList
 	err := r.Client.List(context.Background(), &current, client.MatchingFields{".metadata.controller": applicationSet.Name})
 
@@ -313,7 +430,7 @@ func (r *ApplicationSetReconciler) deleteInCluster(ctx context.Context, applicat
 		appLog := log.WithFields(log.Fields{"app": app.Name, "appSet": applicationSet.Name})
 		_, exists := m[app.Name]
 
-		if exists == false {
+		if !exists {
 			err := r.Client.Delete(ctx, &app)
 			if err != nil {
 				appLog.WithError(err).Error("failed to delete Application")
diff --git a/pkg/generators/cluster.go b/pkg/generators/cluster.go
index 7d15b6c..71b2980 100644
--- a/pkg/generators/cluster.go
+++ b/pkg/generators/cluster.go
@@ -57,7 +57,7 @@ func (g *ClusterGenerator) GenerateParams(
 		return nil, err
 	}
 
-	if err := g.Client.List(context.Background(), clusterSecretList, client.MatchingLabelsSelector{secretSelector}); err != nil {
+	if err := g.Client.List(context.Background(), clusterSecretList, client.MatchingLabelsSelector{Selector: secretSelector}); err != nil {
 		return nil, err
 	}
 	log.Debug("clusters matching labels", "count", len(clusterSecretList.Items))
diff --git a/pkg/generators/git.go b/pkg/generators/git.go
index a37551f..317e69d 100644
--- a/pkg/generators/git.go
+++ b/pkg/generators/git.go
@@ -74,7 +74,8 @@ func (g *GitGenerator) filter(Directories []argoprojiov1alpha1.GitDirectoryGener
 	return res
 }
 
-func (g *GitGenerator) generateParams(requestedApps []string, appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator) []map[string]string {
+func (g *GitGenerator) generateParams(requestedApps []string, _ *argoprojiov1alpha1.ApplicationSetGenerator) []map[string]string {
+	// TODO: At some point, the appicationSetGenerator param should be used
 
 	res := make([]map[string]string, len(requestedApps))
 	for i, a := range requestedApps {
diff --git a/pkg/generators/interface.go b/pkg/generators/interface.go
index e7cc770..8f7798b 100644
--- a/pkg/generators/interface.go
+++ b/pkg/generators/interface.go
@@ -2,8 +2,9 @@ package generators
 
 import (
 	"errors"
-	argoprojiov1alpha1 "github.com/argoproj-labs/applicationset/api/v1alpha1"
 	"time"
+
+	argoprojiov1alpha1 "github.com/argoproj-labs/applicationset/api/v1alpha1"
 )
 
 // Generator defines the interface implemented by all ApplicationSet generators.
diff --git a/pkg/generators/list.go b/pkg/generators/list.go
index 51db54d..af19ce6 100644
--- a/pkg/generators/list.go
+++ b/pkg/generators/list.go
@@ -1,9 +1,10 @@
 package generators
 
 import (
+	"time"
+
 	argoprojiov1alpha1 "github.com/argoproj-labs/applicationset/api/v1alpha1"
 	"github.com/argoproj-labs/applicationset/pkg/utils"
-	"time"
 )
 
 var _ Generator = (*ListGenerator)(nil)
diff --git a/pkg/services/repo_service.go b/pkg/services/repo_service.go
index 166ae99..5e1c998 100644
--- a/pkg/services/repo_service.go
+++ b/pkg/services/repo_service.go
@@ -2,6 +2,7 @@ package services
 
 import (
 	"context"
+
 	"github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1"
 	"github.com/argoproj/argo-cd/reposerver/apiclient"
 	"github.com/argoproj/argo-cd/util/db"
@@ -60,7 +61,7 @@ func (a *argoCDService) GetApps(ctx context.Context, repoURL string, revision st
 
 	res := []string{}
 
-	for name, _ := range apps.Apps {
+	for name := range apps.Apps {
 		res = append(res, name)
 	}
 
diff --git a/pkg/utils/createOrUpdate.go b/pkg/utils/createOrUpdate.go
index 21fee85..7c89c24 100644
--- a/pkg/utils/createOrUpdate.go
+++ b/pkg/utils/createOrUpdate.go
@@ -3,6 +3,7 @@ package utils
 import (
 	"context"
 	"fmt"
+
 	argov1alpha1 "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/resource"
diff --git a/pkg/utils/util.go b/pkg/utils/util.go
index e10f0cb..042b2bc 100644
--- a/pkg/utils/util.go
+++ b/pkg/utils/util.go
@@ -3,11 +3,12 @@ package utils
 import (
 	"encoding/json"
 	"fmt"
+	"io"
+	"strconv"
+
 	argov1alpha1 "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1"
 	"github.com/pkg/errors"
 	"github.com/valyala/fasttemplate"
-	"io"
-	"strconv"
 )
 
 type Renderer interface {


Test patch:
diff --git a/pkg/controllers/applicationset_controller_test.go b/pkg/controllers/applicationset_controller_test.go
index 7146bca..242daf2 100644
--- a/pkg/controllers/applicationset_controller_test.go
+++ b/pkg/controllers/applicationset_controller_test.go
@@ -4,8 +4,13 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"testing"
+	"time"
+
 	"github.com/argoproj-labs/applicationset/pkg/generators"
 	argov1alpha1 "github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1"
+	"github.com/sirupsen/logrus"
+	logtest "github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -14,8 +19,6 @@ import (
 	crtclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
-	"testing"
-	"time"
 
 	argoprojiov1alpha1 "github.com/argoproj-labs/applicationset/api/v1alpha1"
 )
@@ -53,8 +56,11 @@ func (r *rendererMock) RenderTemplateParams(tmpl *argov1alpha1.Application, para
 
 func TestExtractApplications(t *testing.T) {
 	scheme := runtime.NewScheme()
-	argoprojiov1alpha1.AddToScheme(scheme)
-	argov1alpha1.AddToScheme(scheme)
+	err := argoprojiov1alpha1.AddToScheme(scheme)
+	assert.Nil(t, err)
+
+	err = argov1alpha1.AddToScheme(scheme)
+	assert.Nil(t, err)
 
 	client := fake.NewFakeClientWithScheme(scheme)
 
@@ -174,8 +180,11 @@ func TestExtractApplications(t *testing.T) {
 func TestCreateOrUpdateInCluster(t *testing.T) {
 
 	scheme := runtime.NewScheme()
-	argoprojiov1alpha1.AddToScheme(scheme)
-	argov1alpha1.AddToScheme(scheme)
+	err := argoprojiov1alpha1.AddToScheme(scheme)
+	assert.Nil(t, err)
+
+	err = argov1alpha1.AddToScheme(scheme)
+	assert.Nil(t, err)
 
 	for _, c := range []struct {
 		appSet     argoprojiov1alpha1.ApplicationSet
@@ -329,7 +338,8 @@ func TestCreateOrUpdateInCluster(t *testing.T) {
 	} {
 		initObjs := []runtime.Object{&c.appSet}
 		for _, a := range c.existsApps {
-			controllerutil.SetControllerReference(&c.appSet, &a, scheme)
+			err = controllerutil.SetControllerReference(&c.appSet, &a, scheme)
+			assert.Nil(t, err)
 			initObjs = append(initObjs, &a)
 		}
 
@@ -341,7 +351,8 @@ func TestCreateOrUpdateInCluster(t *testing.T) {
 			Recorder: record.NewFakeRecorder(len(initObjs) + len(c.expected)),
 		}
 
-		r.createOrUpdateInCluster(context.TODO(), c.appSet, c.apps)
+		err = r.createOrUpdateInCluster(context.TODO(), c.appSet, c.apps)
+		assert.Nil(t, err)
 
 		for _, obj := range c.expected {
 			got := &argov1alpha1.Application{}
@@ -350,7 +361,8 @@ func TestCreateOrUpdateInCluster(t *testing.T) {
 				Name:      obj.Name,
 			}, got)
 
-			controllerutil.SetControllerReference(&c.appSet, &obj, r.Scheme)
+			err = controllerutil.SetControllerReference(&c.appSet, &obj, r.Scheme)
+			assert.Nil(t, err)
 
 			assert.Equal(t, obj, *got)
 		}
@@ -361,8 +373,11 @@ func TestCreateOrUpdateInCluster(t *testing.T) {
 func TestCreateApplications(t *testing.T) {
 
 	scheme := runtime.NewScheme()
-	argoprojiov1alpha1.AddToScheme(scheme)
-	argov1alpha1.AddToScheme(scheme)
+	err := argoprojiov1alpha1.AddToScheme(scheme)
+	assert.Nil(t, err)
+
+	err = argov1alpha1.AddToScheme(scheme)
+	assert.Nil(t, err)
 
 	for _, c := range []struct {
 		appSet     argoprojiov1alpha1.ApplicationSet
@@ -516,7 +531,8 @@ func TestCreateApplications(t *testing.T) {
 	} {
 		initObjs := []runtime.Object{&c.appSet}
 		for _, a := range c.existsApps {
-			controllerutil.SetControllerReference(&c.appSet, &a, scheme)
+			err = controllerutil.SetControllerReference(&c.appSet, &a, scheme)
+			assert.Nil(t, err)
 			initObjs = append(initObjs, &a)
 		}
 
@@ -528,7 +544,8 @@ func TestCreateApplications(t *testing.T) {
 			Recorder: record.NewFakeRecorder(len(initObjs) + len(c.expected)),
 		}
 
-		r.createInCluster(context.TODO(), c.appSet, c.apps)
+		err = r.createInCluster(context.TODO(), c.appSet, c.apps)
+		assert.Nil(t, err)
 
 		for _, obj := range c.expected {
 			got := &argov1alpha1.Application{}
@@ -537,7 +554,8 @@ func TestCreateApplications(t *testing.T) {
 				Name:      obj.Name,
 			}, got)
 
-			controllerutil.SetControllerReference(&c.appSet, &obj, r.Scheme)
+			err = controllerutil.SetControllerReference(&c.appSet, &obj, r.Scheme)
+			assert.Nil(t, err)
 
 			assert.Equal(t, obj, *got)
 		}
@@ -548,8 +566,10 @@ func TestCreateApplications(t *testing.T) {
 func TestDeleteInCluster(t *testing.T) {
 
 	scheme := runtime.NewScheme()
-	argoprojiov1alpha1.AddToScheme(scheme)
-	argov1alpha1.AddToScheme(scheme)
+	err := argoprojiov1alpha1.AddToScheme(scheme)
+	assert.Nil(t, err)
+	err = argov1alpha1.AddToScheme(scheme)
+	assert.Nil(t, err)
 
 	for _, c := range []struct {
 		appSet      argoprojiov1alpha1.ApplicationSet
@@ -649,7 +669,8 @@ func TestDeleteInCluster(t *testing.T) {
 		initObjs := []runtime.Object{&c.appSet}
 		for _, a := range c.existsApps {
 			temp := a
-			controllerutil.SetControllerReference(&c.appSet, &temp, scheme)
+			err = controllerutil.SetControllerReference(&c.appSet, &temp, scheme)
+			assert.Nil(t, err)
 			initObjs = append(initObjs, &temp)
 		}
 
@@ -661,7 +682,8 @@ func TestDeleteInCluster(t *testing.T) {
 			Recorder: record.NewFakeRecorder(len(initObjs) + len(c.expected)),
 		}
 
-		r.deleteInCluster(context.TODO(), c.appSet, c.apps)
+		err = r.deleteInCluster(context.TODO(), c.appSet, c.apps)
+		assert.Nil(t, err)
 
 		for _, obj := range c.expected {
 			got := &argov1alpha1.Application{}
@@ -670,7 +692,8 @@ func TestDeleteInCluster(t *testing.T) {
 				Name:      obj.Name,
 			}, got)
 
-			controllerutil.SetControllerReference(&c.appSet, &obj, r.Scheme)
+			err = controllerutil.SetControllerReference(&c.appSet, &obj, r.Scheme)
+			assert.Nil(t, err)
 
 			assert.Equal(t, obj, *got)
 		}
@@ -689,8 +712,10 @@ func TestDeleteInCluster(t *testing.T) {
 
 func TestGetMinRequeueAfter(t *testing.T) {
 	scheme := runtime.NewScheme()
-	argoprojiov1alpha1.AddToScheme(scheme)
-	argov1alpha1.AddToScheme(scheme)
+	err := argoprojiov1alpha1.AddToScheme(scheme)
+	assert.Nil(t, err)
+	err = argov1alpha1.AddToScheme(scheme)
+	assert.Nil(t, err)
 
 	client := fake.NewFakeClientWithScheme(scheme)
 
@@ -731,3 +756,438 @@ func TestGetMinRequeueAfter(t *testing.T) {
 
 	assert.Equal(t, time.Duration(1)*time.Second, got)
 }
+
+func TestInvalidGenerators(t *testing.T) {
+
+	scheme := runtime.NewScheme()
+	err := argoprojiov1alpha1.AddToScheme(scheme)
+	assert.Nil(t, err)
+	err = argov1alpha1.AddToScheme(scheme)
+	assert.Nil(t, err)
+
+	for _, c := range []struct {
+		testName        string
+		appSet          argoprojiov1alpha1.ApplicationSet
+		expectedInvalid bool
+		expectedNames   map[string]bool
+	}{
+		{
+			testName: "valid generators, with annotation",
+			appSet: argoprojiov1alpha1.ApplicationSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "name",
+					Namespace: "namespace",
+					Annotations: map[string]string{
+						"kubectl.kubernetes.io/last-applied-configuration": `{
+							"spec":{
+								"generators":[
+									{"list":{}},
+									{"cluster":{}},
+									{"git":{}}
+								]
+							}
+						}`,
+					},
+				},
+				Spec: argoprojiov1alpha1.ApplicationSetSpec{
+					Generators: []argoprojiov1alpha1.ApplicationSetGenerator{
+						{
+							List:     &argoprojiov1alpha1.ListGenerator{},
+							Clusters: nil,
+							Git:      nil,
+						},
+						{
+							List:     nil,
+							Clusters: &argoprojiov1alpha1.ClusterGenerator{},
+							Git:      nil,
+						},
+						{
+							List:     nil,
+							Clusters: nil,
+							Git:      &argoprojiov1alpha1.GitGenerator{},
+						},
+					},
+				},
+			},
+			expectedInvalid: false,
+			expectedNames:   map[string]bool{},
+		},
+		{
+			testName: "invalid generators, no annotation",
+			appSet: argoprojiov1alpha1.ApplicationSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "name",
+					Namespace: "namespace",
+				},
+				Spec: argoprojiov1alpha1.ApplicationSetSpec{
+					Generators: []argoprojiov1alpha1.ApplicationSetGenerator{
+						{
+							List:     nil,
+							Clusters: nil,
+							Git:      nil,
+						},
+						{
+							List:     nil,
+							Clusters: nil,
+							Git:      nil,
+						},
+					},
+				},
+			},
+			expectedInvalid: true,
+			expectedNames:   map[string]bool{},
+		},
+		{
+			testName: "valid and invalid generators, no annotation",
+			appSet: argoprojiov1alpha1.ApplicationSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "name",
+					Namespace: "namespace",
+				},
+				Spec: argoprojiov1alpha1.ApplicationSetSpec{
+					Generators: []argoprojiov1alpha1.ApplicationSetGenerator{
+						{
+							List:     nil,
+							Clusters: &argoprojiov1alpha1.ClusterGenerator{},
+							Git:      nil,
+						},
+						{
+							List:     nil,
+							Clusters: nil,
+							Git:      nil,
+						},
+						{
+							List:     nil,
+							Clusters: nil,
+							Git:      &argoprojiov1alpha1.GitGenerator{},
+						},
+					},
+				},
+			},
+			expectedInvalid: true,
+			expectedNames:   map[string]bool{},
+		},
+		{
+			testName: "valid and invalid generators, with annotation",
+			appSet: argoprojiov1alpha1.ApplicationSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "name",
+					Namespace: "namespace",
+					Annotations: map[string]string{
+						"kubectl.kubernetes.io/last-applied-configuration": `{
+							"spec":{
+								"generators":[
+									{"cluster":{}},
+									{"bbb":{}},
+									{"git":{}},
+									{"aaa":{}}
+								]
+							}
+						}`,
+					},
+				},
+				Spec: argoprojiov1alpha1.ApplicationSetSpec{
+					Generators: []argoprojiov1alpha1.ApplicationSetGenerator{
+						{
+							List:     nil,
+							Clusters: &argoprojiov1alpha1.ClusterGenerator{},
+							Git:      nil,
+						},
+						{
+							List:     nil,
+							Clusters: nil,
+							Git:      nil,
+						},
+						{
+							List:     nil,
+							Clusters: nil,
+							Git:      &argoprojiov1alpha1.GitGenerator{},
+						},
+						{
+							List:     nil,
+							Clusters: nil,
+							Git:      nil,
+						},
+					},
+				},
+			},
+			expectedInvalid: true,
+			expectedNames: map[string]bool{
+				"aaa": true,
+				"bbb": true,
+			},
+		},
+		{
+			testName: "invalid generator, annotation with missing spec",
+			appSet: argoprojiov1alpha1.ApplicationSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "name",
+					Namespace: "namespace",
+					Annotations: map[string]string{
+						"kubectl.kubernetes.io/last-applied-configuration": `{
+						}`,
+					},
+				},
+				Spec: argoprojiov1alpha1.ApplicationSetSpec{
+					Generators: []argoprojiov1alpha1.ApplicationSetGenerator{
+						{
+							List:     nil,
+							Clusters: nil,
+							Git:      nil,
+						},
+					},
+				},
+			},
+			expectedInvalid: true,
+			expectedNames:   map[string]bool{},
+		},
+		{
+			testName: "invalid generator, annotation with missing generators array",
+			appSet: argoprojiov1alpha1.ApplicationSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "name",
+					Namespace: "namespace",
+					Annotations: map[string]string{
+						"kubectl.kubernetes.io/last-applied-configuration": `{
+							"spec":{
+							}
+						}`,
+					},
+				},
+				Spec: argoprojiov1alpha1.ApplicationSetSpec{
+					Generators: []argoprojiov1alpha1.ApplicationSetGenerator{
+						{
+							List:     nil,
+							Clusters: nil,
+							Git:      nil,
+						},
+					},
+				},
+			},
+			expectedInvalid: true,
+			expectedNames:   map[string]bool{},
+		},
+		{
+			testName: "invalid generator, annotation with empty generators array",
+			appSet: argoprojiov1alpha1.ApplicationSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "name",
+					Namespace: "namespace",
+					Annotations: map[string]string{
+						"kubectl.kubernetes.io/last-applied-configuration": `{
+							"spec":{
+								"generators":[
+								]
+							}
+						}`,
+					},
+				},
+				Spec: argoprojiov1alpha1.ApplicationSetSpec{
+					Generators: []argoprojiov1alpha1.ApplicationSetGenerator{
+						{
+							List:     nil,
+							Clusters: nil,
+							Git:      nil,
+						},
+					},
+				},
+			},
+			expectedInvalid: true,
+			expectedNames:   map[string]bool{},
+		},
+		{
+			testName: "invalid generator, annotation with empty generator",
+			appSet: argoprojiov1alpha1.ApplicationSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "name",
+					Namespace: "namespace",
+					Annotations: map[string]string{
+						"kubectl.kubernetes.io/last-applied-configuration": `{
+							"spec":{
+								"generators":[
+									{}
+								]
+							}
+						}`,
+					},
+				},
+				Spec: argoprojiov1alpha1.ApplicationSetSpec{
+					Generators: []argoprojiov1alpha1.ApplicationSetGenerator{
+						{
+							List:     nil,
+							Clusters: nil,
+							Git:      nil,
+						},
+					},
+				},
+			},
+			expectedInvalid: true,
+			expectedNames:   map[string]bool{},
+		},
+	} {
+		hasInvalid, names := invalidGenerators(&c.appSet)
+		assert.Equal(t, c.expectedInvalid, hasInvalid, c.testName)
+		assert.Equal(t, c.expectedNames, names, c.testName)
+	}
+}
+
+func TestCheckInvalidGenerators(t *testing.T) {
+
+	scheme := runtime.NewScheme()
+	err := argoprojiov1alpha1.AddToScheme(scheme)
+	assert.Nil(t, err)
+	err = argov1alpha1.AddToScheme(scheme)
+	assert.Nil(t, err)
+
+	for _, c := range []struct {
+		testName    string
+		appSet      argoprojiov1alpha1.ApplicationSet
+		expectedMsg string
+	}{
+		{
+			testName: "invalid generator, without annotation",
+			appSet: argoprojiov1alpha1.ApplicationSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-app-set",
+					Namespace: "namespace",
+				},
+				Spec: argoprojiov1alpha1.ApplicationSetSpec{
+					Generators: []argoprojiov1alpha1.ApplicationSetGenerator{
+						{
+							List:     &argoprojiov1alpha1.ListGenerator{},
+							Clusters: nil,
+							Git:      nil,
+						},
+						{
+							List:     nil,
+							Clusters: nil,
+							Git:      nil,
+						},
+						{
+							List:     nil,
+							Clusters: nil,
+							Git:      &argoprojiov1alpha1.GitGenerator{},
+						},
+					},
+				},
+			},
+			expectedMsg: "ApplicationSet test-app-set contains unrecognized generators",
+		},
+		{
+			testName: "invalid generator, with annotation",
+			appSet: argoprojiov1alpha1.ApplicationSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-app-set",
+					Namespace: "namespace",
+					Annotations: map[string]string{
+						"kubectl.kubernetes.io/last-applied-configuration": `{
+							"spec":{
+								"generators":[
+									{"list":{}},
+									{"bbb":{}},
+									{"git":{}},
+									{"aaa":{}}
+								]
+							}
+						}`,
+					},
+				},
+				Spec: argoprojiov1alpha1.ApplicationSetSpec{
+					Generators: []argoprojiov1alpha1.ApplicationSetGenerator{
+						{
+							List:     &argoprojiov1alpha1.ListGenerator{},
+							Clusters: nil,
+							Git:      nil,
+						},
+						{
+							List:     nil,
+							Clusters: nil,
+							Git:      nil,
+						},
+						{
+							List:     nil,
+							Clusters: nil,
+							Git:      &argoprojiov1alpha1.GitGenerator{},
+						},
+						{
+							List:     nil,
+							Clusters: nil,
+							Git:      nil,
+						},
+					},
+				},
+			},
+			expectedMsg: "ApplicationSet test-app-set contains unrecognized generators: aaa, bbb",
+		},
+	} {
+		oldhooks := logrus.StandardLogger().ReplaceHooks(logrus.LevelHooks{})
+		defer logrus.StandardLogger().ReplaceHooks(oldhooks)
+		hook := logtest.NewGlobal()
+
+		checkInvalidGenerators(&c.appSet)
+		assert.True(t, len(hook.Entries) >= 1, c.testName)
+		assert.Equal(t, logrus.WarnLevel, hook.LastEntry().Level, c.testName)
+		assert.Equal(t, c.expectedMsg, hook.LastEntry().Message, c.testName)
+		hook.Reset()
+	}
+}
+
+func TestHasDuplicateNames(t *testing.T) {
+
+	scheme := runtime.NewScheme()
+	err := argoprojiov1alpha1.AddToScheme(scheme)
+	assert.Nil(t, err)
+	err = argov1alpha1.AddToScheme(scheme)
+	assert.Nil(t, err)
+
+	for _, c := range []struct {
+		testName      string
+		desiredApps   []argov1alpha1.Application
+		hasDuplicates bool
+		duplicateName string
+	}{
+		{
+			testName: "has no duplicates",
+			desiredApps: []argov1alpha1.Application{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "app1",
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "app2",
+					},
+				},
+			},
+			hasDuplicates: false,
+			duplicateName: "",
+		},
+		{
+			testName: "has duplicates",
+			desiredApps: []argov1alpha1.Application{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "app1",
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "app2",
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "app1",
+					},
+				},
+			},
+			hasDuplicates: true,
+			duplicateName: "app1",
+		},
+	} {
+		hasDuplicates, name := hasDuplicateNames(c.desiredApps)
+		assert.Equal(t, c.hasDuplicates, hasDuplicates)
+		assert.Equal(t, c.duplicateName, name)
+	}
+}
diff --git a/pkg/generators/git_test.go b/pkg/generators/git_test.go
index 17f66ee..7936e54 100644
--- a/pkg/generators/git_test.go
+++ b/pkg/generators/git_test.go
@@ -3,29 +3,28 @@ package generators
 import (
 	"context"
 	"fmt"
+	"testing"
+
 	argoprojiov1alpha1 "github.com/argoproj-labs/applicationset/api/v1alpha1"
-	"github.com/argoproj/argo-cd/reposerver/apiclient"
-	"github.com/argoproj/gitops-engine/pkg/utils/io"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"testing"
 )
 
-type clientSet struct {
-	RepoServerServiceClient apiclient.RepoServerServiceClient
-}
+// type clientSet struct {
+// 	RepoServerServiceClient apiclient.RepoServerServiceClient
+// }
 
-func (c *clientSet) NewRepoServerClient() (io.Closer, apiclient.RepoServerServiceClient, error) {
-	return io.NewCloser(func() error { return nil }), c.RepoServerServiceClient, nil
-}
+// func (c *clientSet) NewRepoServerClient() (io.Closer, apiclient.RepoServerServiceClient, error) {
+// 	return io.NewCloser(func() error { return nil }), c.RepoServerServiceClient, nil
+// }
 
 type argoCDServiceMock struct {
-	mock.Mock
+	mock *mock.Mock
 }
 
 func (a argoCDServiceMock) GetApps(ctx context.Context, repoURL string, revision string) ([]string, error) {
-	args := a.Called(ctx, repoURL, revision)
+	args := a.mock.Called(ctx, repoURL, revision)
 
 	return args.Get(0).([]string), args.Error(1)
 }
@@ -42,7 +41,7 @@ func TestGitGenerateParams(t *testing.T) {
 	}{
 		{
 			name:        "happy flow - created apps",
-			directories: []argoprojiov1alpha1.GitDirectoryGeneratorItem{{"*"}},
+			directories: []argoprojiov1alpha1.GitDirectoryGeneratorItem{{Path: "*"}},
 			repoApps: []string{
 				"app1",
 				"app2",
@@ -57,7 +56,7 @@ func TestGitGenerateParams(t *testing.T) {
 		},
 		{
 			name:        "It filters application according to the paths",
-			directories: []argoprojiov1alpha1.GitDirectoryGeneratorItem{{"p1/*"}, {"p1/*/*"}},
+			directories: []argoprojiov1alpha1.GitDirectoryGeneratorItem{{Path: "p1/*"}, {Path: "p1/*/*"}},
 			repoApps: []string{
 				"app1",
 				"p1/app2",
@@ -73,7 +72,7 @@ func TestGitGenerateParams(t *testing.T) {
 		},
 		{
 			name:          "handles empty response from repo server",
-			directories:   []argoprojiov1alpha1.GitDirectoryGeneratorItem{{"*"}},
+			directories:   []argoprojiov1alpha1.GitDirectoryGeneratorItem{{Path: "*"}},
 			repoApps:      []string{},
 			repoError:     nil,
 			expected:      []map[string]string{},
@@ -81,7 +80,7 @@ func TestGitGenerateParams(t *testing.T) {
 		},
 		{
 			name:          "handles error from repo server",
-			directories:   []argoprojiov1alpha1.GitDirectoryGeneratorItem{{"*"}},
+			directories:   []argoprojiov1alpha1.GitDirectoryGeneratorItem{{Path: "*"}},
 			repoApps:      []string{},
 			repoError:     fmt.Errorf("error"),
 			expected:      []map[string]string{},
@@ -92,8 +91,8 @@ func TestGitGenerateParams(t *testing.T) {
 	for _, c := range cases {
 		cc := c
 		t.Run(cc.name, func(t *testing.T) {
-			argoCDServiceMock := argoCDServiceMock{}
-			argoCDServiceMock.On("GetApps", mock.Anything, mock.Anything, mock.Anything).Return(c.repoApps, c.repoError)
+			argoCDServiceMock := argoCDServiceMock{mock: &mock.Mock{}}
+			argoCDServiceMock.mock.On("GetApps", mock.Anything, mock.Anything, mock.Anything).Return(c.repoApps, c.repoError)
 
 			var gitGenerator = NewGitGenerator(argoCDServiceMock)
 			applicationSetInfo := argoprojiov1alpha1.ApplicationSet{
@@ -120,7 +119,7 @@ func TestGitGenerateParams(t *testing.T) {
 				assert.Equal(t, c.expected, got)
 			}
 
-			argoCDServiceMock.AssertExpectations(t)
+			argoCDServiceMock.mock.AssertExpectations(t)
 		})
 	}
 
diff --git a/pkg/services/repo_service_test.go b/pkg/services/repo_service_test.go
index 3c6d038..81ef66c 100644
--- a/pkg/services/repo_service_test.go
+++ b/pkg/services/repo_service_test.go
@@ -3,35 +3,36 @@ package services
 import (
 	"context"
 	"errors"
+	"testing"
+
 	"github.com/argoproj/argo-cd/pkg/apis/application/v1alpha1"
 	"github.com/argoproj/argo-cd/reposerver/apiclient"
 	"github.com/argoproj/gitops-engine/pkg/utils/io"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	"google.golang.org/grpc"
-	"testing"
 )
 
 type ArgocdRepositoryMock struct {
-	mock.Mock
+	mock *mock.Mock
 }
 
 func (a ArgocdRepositoryMock) GetRepository(ctx context.Context, url string) (*v1alpha1.Repository, error) {
-	args := a.Called(ctx, url)
+	args := a.mock.Called(ctx, url)
 
 	return args.Get(0).(*v1alpha1.Repository), args.Error(1)
 
 }
 
 type repoServerClientMock struct {
-	mock.Mock
+	mock *mock.Mock
 }
 
 func (r repoServerClientMock) GenerateManifest(ctx context.Context, in *apiclient.ManifestRequest, opts ...grpc.CallOption) (*apiclient.ManifestResponse, error) {
 	return nil, nil
 }
 func (r repoServerClientMock) ListApps(ctx context.Context, in *apiclient.ListAppsRequest, opts ...grpc.CallOption) (*apiclient.AppList, error) {
-	args := r.Called(ctx, in)
+	args := r.mock.Called(ctx, in)
 
 	return args.Get(0).(*apiclient.AppList), args.Error(1)
 }
@@ -46,7 +47,7 @@ func (r repoServerClientMock) GetHelmCharts(ctx context.Context, in *apiclient.H
 }
 
 type closer struct {
-	mock.Mock
+	// mock *mock.Mock
 }
 
 func (c closer) Close() error {
@@ -54,11 +55,11 @@ func (c closer) Close() error {
 }
 
 type repoClientsetMock struct {
-	mock.Mock
+	mock *mock.Mock
 }
 
 func (r repoClientsetMock) NewRepoServerClient() (io.Closer, apiclient.RepoServerServiceClient, error) {
-	args := r.Called()
+	args := r.mock.Called()
 
 	return closer{}, args.Get(0).(apiclient.RepoServerServiceClient), args.Error(1)
 }
@@ -127,18 +128,18 @@ func TestGetApps(t *testing.T) {
 	} {
 		cc := c
 		t.Run(cc.name, func(t *testing.T) {
-			argocdRepositoryMock := ArgocdRepositoryMock{}
-			repoServerClientMock := repoServerClientMock{}
-			repoClientsetMock := repoClientsetMock{}
+			argocdRepositoryMock := ArgocdRepositoryMock{mock: &mock.Mock{}}
+			repoServerClientMock := repoServerClientMock{mock: &mock.Mock{}}
+			repoClientsetMock := repoClientsetMock{mock: &mock.Mock{}}
 
-			argocdRepositoryMock.On("GetRepository", mock.Anything, cc.repoURL).Return(cc.repoRes, cc.repoErr)
+			argocdRepositoryMock.mock.On("GetRepository", mock.Anything, cc.repoURL).Return(cc.repoRes, cc.repoErr)
 
-			repoServerClientMock.On("ListApps", mock.Anything, &apiclient.ListAppsRequest{
+			repoServerClientMock.mock.On("ListApps", mock.Anything, &apiclient.ListAppsRequest{
 				Repo:     cc.repoRes,
 				Revision: cc.revision,
 			}).Return(cc.appRes, cc.appError)
 
-			repoClientsetMock.On("NewRepoServerClient").Return(repoServerClientMock, nil)
+			repoClientsetMock.mock.On("NewRepoServerClient").Return(repoServerClientMock, nil)
 
 			argocd := argoCDService{
 				repositoriesDB: argocdRepositoryMock,