Fixing up some code smells

Signed-off-by: Gustavo Carvalho <gustavo.carvalho@container-solutions.com>

apis/externalsecrets/v1alpha1/externalsecret_conversion_test.go

@@ -24,6 +24,10 @@ import (
 	esv1beta1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
 )
 
+const (
+	keyName = "my-key"
+)
+
 func newExternalSecretV1Alpha1() *ExternalSecret {
 	return &ExternalSecret{
 		ObjectMeta: metav1.ObjectMeta{
@@ -69,7 +73,7 @@ func newExternalSecretV1Alpha1() *ExternalSecret {
 								Name: "test-configmap",
 								Items: []TemplateRefItem{
 									{
-										Key: "my-key",
+										Key: keyName,
 									},
 								},
 							},
@@ -77,7 +81,7 @@ func newExternalSecretV1Alpha1() *ExternalSecret {
 								Name: "test-secret",
 								Items: []TemplateRefItem{
 									{
-										Key: "my-key",
+										Key: keyName,
 									},
 								},
 							},
@@ -87,7 +91,7 @@ func newExternalSecretV1Alpha1() *ExternalSecret {
 			},
 			Data: []ExternalSecretData{
 				{
-					SecretKey: "my-key",
+					SecretKey: keyName,
 					RemoteRef: ExternalSecretDataRemoteRef{
 						Key:      "datakey",
 						Property: "dataproperty",
@@ -151,7 +155,7 @@ func newExternalSecretV1Beta1() *esv1beta1.ExternalSecret {
 								Name: "test-configmap",
 								Items: []esv1beta1.TemplateRefItem{
 									{
-										Key: "my-key",
+										Key: keyName,
 									},
 								},
 							},
@@ -159,7 +163,7 @@ func newExternalSecretV1Beta1() *esv1beta1.ExternalSecret {
 								Name: "test-secret",
 								Items: []esv1beta1.TemplateRefItem{
 									{
-										Key: "my-key",
+										Key: keyName,
 									},
 								},
 							},
@@ -169,7 +173,7 @@ func newExternalSecretV1Beta1() *esv1beta1.ExternalSecret {
 			},
 			Data: []esv1beta1.ExternalSecretData{
 				{
-					SecretKey: "my-key",
+					SecretKey: keyName,
 					RemoteRef: esv1beta1.ExternalSecretDataRemoteRef{
 						Key:      "datakey",
 						Property: "dataproperty",

apis/externalsecrets/v1alpha1/secretstore_conversion_test.go

@@ -25,19 +25,33 @@ import (
 	esmeta "github.com/external-secrets/external-secrets/apis/meta/v1"
 )
 
+const (
+	storeName                = "secret-store"
+	storeNamespace           = "my-namespace"
+	storeReason              = "it's a mock, it's always ready"
+	storeMessage             = "...why wouldn't it be?"
+	storeAWSRegion           = "us-east-1"
+	storeAWSRole             = "arn:aws:iam::123456789012:role/my-role"
+	storeAccessName          = "my-access"
+	storeKey                 = "my-key"
+	storeSecretName          = "my-secret"
+	defaultErrorMessage      = "test failed with error: %v"
+	defaultComparisonMessage = "test failed, expected: %v, got: %v"
+)
+
 func newSecretStoreV1Alpha1() *SecretStore {
 	return &SecretStore{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      "secret-store",
-			Namespace: "my-namespace",
+			Name:      storeName,
+			Namespace: storeNamespace,
 		},
 		Status: SecretStoreStatus{
 			Conditions: []SecretStoreStatusCondition{
 				{
 					Type:    SecretStoreReady,
 					Status:  corev1.ConditionTrue,
-					Reason:  "it's a mock, it's always ready",
-					Message: "...why wouldn't it be?",
+					Reason:  storeReason,
+					Message: storeMessage,
 				},
 			},
 		},
@@ -46,17 +60,17 @@ func newSecretStoreV1Alpha1() *SecretStore {
 			Provider: &SecretStoreProvider{
 				AWS: &AWSProvider{
 					Service: AWSServiceSecretsManager,
-					Region:  "us-east-1",
-					Role:    "arn:aws:iam::123456789012:role/my-role",
+					Region:  storeAWSRegion,
+					Role:    storeAWSRole,
 					Auth: AWSAuth{
 						SecretRef: &AWSAuthSecretRef{
 							AccessKeyID: esmeta.SecretKeySelector{
-								Name: "my-access",
-								Key:  "my-key",
+								Name: storeAccessName,
+								Key:  storeKey,
 							},
 							SecretAccessKey: esmeta.SecretKeySelector{
-								Name: "my-secret",
-								Key:  "my-key",
+								Name: storeSecretName,
+								Key:  storeKey,
 							},
 						},
 					},
@@ -69,16 +83,16 @@ func newSecretStoreV1Alpha1() *SecretStore {
 func newSecretStoreV1Beta1() *esv1beta1.SecretStore {
 	return &esv1beta1.SecretStore{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      "secret-store",
-			Namespace: "my-namespace",
+			Name:      storeName,
+			Namespace: storeNamespace,
 		},
 		Status: esv1beta1.SecretStoreStatus{
 			Conditions: []esv1beta1.SecretStoreStatusCondition{
 				{
 					Type:    esv1beta1.SecretStoreReady,
 					Status:  corev1.ConditionTrue,
-					Reason:  "it's a mock, it's always ready",
-					Message: "...why wouldn't it be?",
+					Reason:  storeReason,
+					Message: storeMessage,
 				},
 			},
 		},
@@ -87,17 +101,17 @@ func newSecretStoreV1Beta1() *esv1beta1.SecretStore {
 			Provider: &esv1beta1.SecretStoreProvider{
 				AWS: &esv1beta1.AWSProvider{
 					Service: esv1beta1.AWSServiceSecretsManager,
-					Region:  "us-east-1",
-					Role:    "arn:aws:iam::123456789012:role/my-role",
+					Region:  storeAWSRegion,
+					Role:    storeAWSRole,
 					Auth: esv1beta1.AWSAuth{
 						SecretRef: &esv1beta1.AWSAuthSecretRef{
 							AccessKeyID: esmeta.SecretKeySelector{
-								Name: "my-access",
-								Key:  "my-key",
+								Name: storeAccessName,
+								Key:  storeKey,
 							},
 							SecretAccessKey: esmeta.SecretKeySelector{
-								Name: "my-secret",
-								Key:  "my-key",
+								Name: storeSecretName,
+								Key:  storeKey,
 							},
 						},
 					},
@@ -108,18 +122,18 @@ func newSecretStoreV1Beta1() *esv1beta1.SecretStore {
 }
 
 func newClusterSecretStoreV1Alpha1() *ClusterSecretStore {
-	ns := "my-namespace"
+	ns := storeNamespace
 	return &ClusterSecretStore{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "secret-store",
+			Name: storeName,
 		},
 		Status: SecretStoreStatus{
 			Conditions: []SecretStoreStatusCondition{
 				{
 					Type:    SecretStoreReady,
 					Status:  corev1.ConditionTrue,
-					Reason:  "it's a mock, it's always ready",
-					Message: "...why wouldn't it be?",
+					Reason:  storeReason,
+					Message: storeMessage,
 				},
 			},
 		},
@@ -128,18 +142,18 @@ func newClusterSecretStoreV1Alpha1() *ClusterSecretStore {
 			Provider: &SecretStoreProvider{
 				AWS: &AWSProvider{
 					Service: AWSServiceSecretsManager,
-					Region:  "us-east-1",
-					Role:    "arn:aws:iam::123456789012:role/my-role",
+					Region:  storeAWSRegion,
+					Role:    storeAWSRole,
 					Auth: AWSAuth{
 						SecretRef: &AWSAuthSecretRef{
 							AccessKeyID: esmeta.SecretKeySelector{
-								Name:      "my-access",
-								Key:       "my-key",
+								Name:      storeAccessName,
+								Key:       storeKey,
 								Namespace: &ns,
 							},
 							SecretAccessKey: esmeta.SecretKeySelector{
-								Name:      "my-secret",
-								Key:       "my-key",
+								Name:      storeSecretName,
+								Key:       storeKey,
 								Namespace: &ns,
 							},
 						},
@@ -151,18 +165,18 @@ func newClusterSecretStoreV1Alpha1() *ClusterSecretStore {
 }
 
 func newClusterSecretStoreV1Beta1() *esv1beta1.ClusterSecretStore {
-	ns := "my-namespace"
+	ns := storeNamespace
 	return &esv1beta1.ClusterSecretStore{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "secret-store",
+			Name: storeName,
 		},
 		Status: esv1beta1.SecretStoreStatus{
 			Conditions: []esv1beta1.SecretStoreStatusCondition{
 				{
 					Type:    esv1beta1.SecretStoreReady,
 					Status:  corev1.ConditionTrue,
-					Reason:  "it's a mock, it's always ready",
-					Message: "...why wouldn't it be?",
+					Reason:  storeReason,
+					Message: storeMessage,
 				},
 			},
 		},
@@ -171,18 +185,18 @@ func newClusterSecretStoreV1Beta1() *esv1beta1.ClusterSecretStore {
 			Provider: &esv1beta1.SecretStoreProvider{
 				AWS: &esv1beta1.AWSProvider{
 					Service: esv1beta1.AWSServiceSecretsManager,
-					Region:  "us-east-1",
-					Role:    "arn:aws:iam::123456789012:role/my-role",
+					Region:  storeAWSRegion,
+					Role:    storeAWSRole,
 					Auth: esv1beta1.AWSAuth{
 						SecretRef: &esv1beta1.AWSAuthSecretRef{
 							AccessKeyID: esmeta.SecretKeySelector{
-								Name:      "my-access",
-								Key:       "my-key",
+								Name:      storeAccessName,
+								Key:       storeKey,
 								Namespace: &ns,
 							},
 							SecretAccessKey: esmeta.SecretKeySelector{
-								Name:      "my-secret",
-								Key:       "my-key",
+								Name:      storeSecretName,
+								Key:       storeKey,
 								Namespace: &ns,
 							},
 						},
@@ -198,7 +212,7 @@ func TestSecretStoreConvertFrom(t *testing.T) {
 	got := &SecretStore{}
 	err := got.ConvertFrom(given)
 	if err != nil {
-		t.Errorf("test failed with error: %v", err)
+		t.Errorf(defaultErrorMessage, err)
 	}
 	if !assert.Equal(t, want, got) {
 		t.Errorf("test failed, expected: %v, got: %v", want, got)
@@ -211,10 +225,10 @@ func TestSecretStoreConvertTo(t *testing.T) {
 	got := &esv1beta1.SecretStore{}
 	err := given.ConvertTo(got)
 	if err != nil {
-		t.Errorf("test failed with error: %v", err)
+		t.Errorf(defaultErrorMessage, err)
 	}
 	if !assert.Equal(t, want, got) {
-		t.Errorf("test failed, expected: %v, got: %v", want, got)
+		t.Errorf(defaultComparisonMessage, want, got)
 	}
 }
 
@@ -224,10 +238,10 @@ func TestClusterSecretStoreConvertFrom(t *testing.T) {
 	got := &ClusterSecretStore{}
 	err := got.ConvertFrom(given)
 	if err != nil {
-		t.Errorf("test failed with error: %v", err)
+		t.Errorf(defaultErrorMessage, err)
 	}
 	if !assert.Equal(t, want, got) {
-		t.Errorf("test failed, expected: %v, got: %v", want, got)
+		t.Errorf(defaultComparisonMessage, want, got)
 	}
 }
 
@@ -237,9 +251,9 @@ func TestClusterSecretStoreConvertTo(t *testing.T) {
 	got := &esv1beta1.ClusterSecretStore{}
 	err := given.ConvertTo(got)
 	if err != nil {
-		t.Errorf("test failed with error: %v", err)
+		t.Errorf(defaultErrorMessage, err)
 	}
 	if !assert.Equal(t, want, got) {
-		t.Errorf("test failed, expected: %v, got: %v", want, got)
+		t.Errorf(defaultComparisonMessage, want, got)
 	}
 }

apis/externalsecrets/v1beta1/externalsecret_conversion.go

@@ -14,4 +14,6 @@ limitations under the License.
 
 package v1beta1
 
-func (*ExternalSecret) Hub() {}
+func (*ExternalSecret) Hub() {
+	// This empty method defines the Hub convertible interface.
+}

pkg/controllers/crds/common_test.go

@@ -28,6 +28,12 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 )
 
+const (
+	crdGroup   = "apiextensions.k8s.io"
+	crdKind    = "CustomResourceDefinition"
+	crdVersion = "v1"
+)
+
 type testCase struct {
 	crd     unstructured.Unstructured
 	crd2    unstructured.Unstructured
@@ -44,6 +50,7 @@ var _ = Describe("CRD reconcile", func() {
 	})
 
 	AfterEach(func() {
+		// To improve later on with proper clean up.
 	})
 
 	// a invalid provider config should be reflected
@@ -52,7 +59,7 @@ var _ = Describe("CRD reconcile", func() {
 		tc.assert = func() {
 			Consistently(func() bool {
 				ss := unstructured.Unstructured{}
-				ss.SetGroupVersionKind(schema.GroupVersionKind{Kind: "CustomResourceDefinition", Version: "v1", Group: "apiextensions.k8s.io"})
+				ss.SetGroupVersionKind(schema.GroupVersionKind{Kind: crdKind, Version: crdVersion, Group: crdGroup})
 				err := k8sClient.Get(context.Background(), types.NamespacedName{
 					Name: "secretstores.test.io",
 				}, &ss)
@@ -81,7 +88,7 @@ var _ = Describe("CRD reconcile", func() {
 		tc.assert = func() {
 			Consistently(func() bool {
 				ss := unstructured.Unstructured{}
-				ss.SetGroupVersionKind(schema.GroupVersionKind{Kind: "CustomResourceDefinition", Version: "v1", Group: "apiextensions.k8s.io"})
+				ss.SetGroupVersionKind(schema.GroupVersionKind{Kind: crdKind, Version: crdVersion, Group: crdGroup})
 				err := k8sClient.Get(context.Background(), types.NamespacedName{
 					Name: "some-other.test.io",
 				}, &ss)
@@ -169,7 +176,7 @@ func makeUnstructuredCRD(plural, group string) unstructured.Unstructured {
 	u := unstructured.Unstructured{
 		Object: unmarshal,
 	}
-	u.SetGroupVersionKind(schema.GroupVersionKind{Kind: "CustomResourceDefinition", Version: "v1", Group: "apiextensions.k8s.io"})
+	u.SetGroupVersionKind(schema.GroupVersionKind{Kind: crdKind, Version: "v1", Group: crdGroup})
 	return u
 }
 

pkg/controllers/crds/crds_controller_test.go

@@ -31,6 +31,14 @@ import (
 	client "sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
+const (
+	setupError              = "Could not setup test"
+	errorSearchingField     = "Error when searching for field"
+	failedCreateCaCerts     = "could not create ca certificates:%v"
+	failedCreateServerCerts = "could not create server certificates:%v"
+	invalidCerts            = "generated certificates are invalid:%v,%v"
+)
+
 func newReconciler() Reconciler {
 	return Reconciler{
 		CrdResources:    []string{"one", "two", "three"},
@@ -123,11 +131,11 @@ func TestInjectSvcToConversionWebhook(t *testing.T) {
 	crdunmarshalled := make(map[string]interface{})
 	crdJSON, err := json.Marshal(crd)
 	if err != nil {
-		t.Fatal("Could not setup test")
+		t.Fatal(setupError)
 	}
 	err = json.Unmarshal(crdJSON, &crdunmarshalled)
 	if err != nil {
-		t.Fatal("Could not setup test")
+		t.Fatal(setupError)
 	}
 	u := unstructured.Unstructured{
 		Object: crdunmarshalled,
@@ -142,10 +150,10 @@ func TestInjectSvcToConversionWebhook(t *testing.T) {
 	}
 	val, found, err := unstructured.NestedString(u.Object, "spec", "conversion", "webhook", "clientConfig", "service", "name")
 	if err != nil {
-		t.Error("Error when searching for field")
+		t.Error(errorSearchingField)
 	}
 	if !found {
-		t.Error("Field not found, mutation failed")
+		t.Error("fieldNotFound")
 	}
 	if val != "foo" {
 		t.Errorf("Wrong service name injected: %v", val)
@@ -153,10 +161,10 @@ func TestInjectSvcToConversionWebhook(t *testing.T) {
 
 	val, found, err = unstructured.NestedString(u.Object, "spec", "conversion", "webhook", "clientConfig", "service", "namespace")
 	if err != nil {
-		t.Error("Error when searching for field")
+		t.Error(errorSearchingField)
 	}
 	if !found {
-		t.Error("Field not found, mutation failed")
+		t.Error("fieldNotFound")
 	}
 	if val != "default" {
 		t.Errorf("Wrong service namespace injected: %v", val)
@@ -169,11 +177,11 @@ func TestInjectCertToConversionWebhook(t *testing.T) {
 	crdunmarshalled := make(map[string]interface{})
 	crdJSON, err := json.Marshal(crd)
 	if err != nil {
-		t.Fatal("Could not setup test")
+		t.Fatal(setupError)
 	}
 	err = json.Unmarshal(crdJSON, &crdunmarshalled)
 	if err != nil {
-		t.Fatal("Could not setup test")
+		t.Fatal(setupError)
 	}
 	u := unstructured.Unstructured{
 		Object: crdunmarshalled,
@@ -184,10 +192,10 @@ func TestInjectCertToConversionWebhook(t *testing.T) {
 	}
 	val, found, err := unstructured.NestedString(u.Object, "spec", "conversion", "webhook", "clientConfig", "caBundle")
 	if err != nil {
-		t.Error("Error when searching for field")
+		t.Error(errorSearchingField)
 	}
 	if !found {
-		t.Error("Field not found, mutation failed")
+		t.Error("fieldNotFound")
 	}
 	if val != "Zm9vYmFy" {
 		t.Errorf("Wrong certificate name injected: %v", val)
@@ -222,10 +230,10 @@ func TestCreateCACert(t *testing.T) {
 	rec := newReconciler()
 	caArtifacts, err := rec.CreateCACert(time.Now(), time.Now().AddDate(1, 0, 0))
 	if err != nil {
-		t.Errorf("could not create certificates:%v", err)
+		t.Errorf(failedCreateCaCerts, err)
 	}
 	if !rec.validCACert(caArtifacts.CertPEM, caArtifacts.KeyPEM) {
-		t.Errorf("generated certificates are invalid:%v", caArtifacts)
+		t.Errorf(invalidCerts, caArtifacts.CertPEM, caArtifacts.KeyPEM)
 	}
 }
 
@@ -233,14 +241,14 @@ func TestCreateCertPEM(t *testing.T) {
 	rec := newReconciler()
 	caArtifacts, err := rec.CreateCACert(time.Now(), time.Now().AddDate(1, 0, 0))
 	if err != nil {
-		t.Fatalf("could not create ca certificates:%v", err)
+		t.Fatalf(failedCreateCaCerts, err)
 	}
 	certPEM, keyPEM, err := rec.CreateCertPEM(caArtifacts, time.Now(), time.Now().AddDate(1, 0, 0))
 	if err != nil {
-		t.Errorf("could not create server certificates: %v", err)
+		t.Errorf(failedCreateServerCerts, err)
 	}
 	if !rec.validServerCert(caArtifacts.CertPEM, certPEM, keyPEM) {
-		t.Errorf("generated certificates are invalid:%v,%v", certPEM, keyPEM)
+		t.Errorf(invalidCerts, certPEM, keyPEM)
 	}
 }
 func TestValidCert(t *testing.T) {
@@ -248,11 +256,11 @@ func TestValidCert(t *testing.T) {
 	rec.dnsName = "foobar"
 	caArtifacts, err := rec.CreateCACert(time.Now(), time.Now().AddDate(1, 0, 0))
 	if err != nil {
-		t.Fatalf("could not create ca certificates:%v", err)
+		t.Fatalf(failedCreateCaCerts, err)
 	}
 	certPEM, keyPEM, err := rec.CreateCertPEM(caArtifacts, time.Now(), time.Now().AddDate(1, 0, 0))
 	if err != nil {
-		t.Errorf("could not create server certificates: %v", err)
+		t.Errorf(failedCreateServerCerts, err)
 	}
 	ok, err := ValidCert(caArtifacts.CertPEM, certPEM, keyPEM, "foobar", time.Now())
 	if err != nil {
@@ -271,11 +279,11 @@ func TestRefreshCertIfNeeded(t *testing.T) {
 	rec.dnsName = "foobar"
 	caArtifacts, err := rec.CreateCACert(time.Now().AddDate(-1, 0, 0), time.Now().AddDate(0, -1, 0))
 	if err != nil {
-		t.Fatalf("could not create ca certificates:%v", err)
+		t.Fatalf(failedCreateCaCerts, err)
 	}
 	certPEM, keyPEM, err := rec.CreateCertPEM(caArtifacts, time.Now(), time.Now().AddDate(1, 0, 0))
 	if err != nil {
-		t.Errorf("could not create server certificates: %v", err)
+		t.Errorf(failedCreateServerCerts, err)
 	}
 	populateSecret(certPEM, keyPEM, caArtifacts, &secret)
 	ok, err := rec.refreshCertIfNeeded(&secret)