🌱 Refactor code to fix lint warning (#218)

* Refactor code to fix lint warning Signed-off-by: Jian Qiu <[email protected]> * enable lint for testing files Signed-off-by: Jian Qiu <[email protected]> --------- Signed-off-by: Jian Qiu <[email protected]>
open-cluster-management-io · Jul 25, 2023 · e810520 · e810520
1 parent e22faa4
commit e810520
Show file tree

Hide file tree

Showing 145 changed files with 1,218 additions and 935 deletions.
diff --git a/.golangci.yaml b/.golangci.yaml
@@ -23,7 +23,6 @@ run:
   skip-files:
     - ".*\\.pb\\.go"
     - ".*\\.gen\\.go"
-    - ".*_test\\.go"
 
 linters:
   # please, do not use `enable-all`: it's deprecated and will be removed soon.
@@ -220,6 +219,7 @@ issues:
       linters:
         - errcheck
         - maligned
+        - goconst
 
   # Independently from option `exclude` we use default exclude patterns,
   # it can be disabled by this option. To list all

diff --git a/pkg/addon/controllers/addonconfiguration/addon_configuration_reconciler_test.go b/pkg/addon/controllers/addonconfiguration/addon_configuration_reconciler_test.go
@@ -609,7 +609,10 @@ func (a byPatchName) Less(i, j int) bool {
 	return patchi.Namespace < patchj.Namespace
 }
 
-func newManagedClusterAddon(name, namespace string, configs []addonv1alpha1.AddOnConfig, configStatus []addonv1alpha1.ConfigReference) *addonv1alpha1.ManagedClusterAddOn {
+func newManagedClusterAddon(
+	name, namespace string,
+	configs []addonv1alpha1.AddOnConfig,
+	configStatus []addonv1alpha1.ConfigReference) *addonv1alpha1.ManagedClusterAddOn {
 	mca := addontesting.NewAddon(name, namespace)
 	mca.Spec.Configs = configs
 	mca.Status.ConfigReferences = configStatus

diff --git a/pkg/addon/controllers/addonconfiguration/mgmt_addon_progressing_reconciler_test.go b/pkg/addon/controllers/addonconfiguration/mgmt_addon_progressing_reconciler_test.go
@@ -240,10 +240,14 @@ func TestMgmtAddonProgressingReconcile(t *testing.T) {
 				if len(cma.Status.DefaultConfigReferences) != 0 {
 					t.Errorf("DefaultConfigReferences object is not correct: %v", cma.Status.DefaultConfigReferences)
 				}
-				if !apiequality.Semantic.DeepEqual(cma.Status.InstallProgressions[0].ConfigReferences[0].LastAppliedConfig, cma.Status.InstallProgressions[0].ConfigReferences[0].DesiredConfig) {
+				if !apiequality.Semantic.DeepEqual(
+					cma.Status.InstallProgressions[0].ConfigReferences[0].LastAppliedConfig,
+					cma.Status.InstallProgressions[0].ConfigReferences[0].DesiredConfig) {
 					t.Errorf("InstallProgressions LastAppliedConfig is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences[0])
 				}
-				if !apiequality.Semantic.DeepEqual(cma.Status.InstallProgressions[0].ConfigReferences[0].LastKnownGoodConfig, cma.Status.InstallProgressions[0].ConfigReferences[0].DesiredConfig) {
+				if !apiequality.Semantic.DeepEqual(
+					cma.Status.InstallProgressions[0].ConfigReferences[0].LastKnownGoodConfig,
+					cma.Status.InstallProgressions[0].ConfigReferences[0].DesiredConfig) {
 					t.Errorf("InstallProgressions LastKnownGoodConfig is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences[0])
 				}
 				if cma.Status.InstallProgressions[0].Conditions[0].Reason != addonv1alpha1.ProgressingReasonInstallSucceed {
@@ -389,10 +393,14 @@ func TestMgmtAddonProgressingReconcile(t *testing.T) {
 				if len(cma.Status.DefaultConfigReferences) != 0 {
 					t.Errorf("DefaultConfigReferences object is not correct: %v", cma.Status.DefaultConfigReferences)
 				}
-				if !apiequality.Semantic.DeepEqual(cma.Status.InstallProgressions[0].ConfigReferences[0].LastAppliedConfig, cma.Status.InstallProgressions[0].ConfigReferences[0].DesiredConfig) {
+				if !apiequality.Semantic.DeepEqual(
+					cma.Status.InstallProgressions[0].ConfigReferences[0].LastAppliedConfig,
+					cma.Status.InstallProgressions[0].ConfigReferences[0].DesiredConfig) {
 					t.Errorf("InstallProgressions LastAppliedConfig is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences[0])
 				}
-				if !apiequality.Semantic.DeepEqual(cma.Status.InstallProgressions[0].ConfigReferences[0].LastKnownGoodConfig, cma.Status.InstallProgressions[0].ConfigReferences[0].DesiredConfig) {
+				if !apiequality.Semantic.DeepEqual(
+					cma.Status.InstallProgressions[0].ConfigReferences[0].LastKnownGoodConfig,
+					cma.Status.InstallProgressions[0].ConfigReferences[0].DesiredConfig) {
 					t.Errorf("InstallProgressions LastKnownGoodConfig is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences[0])
 				}
 				if cma.Status.InstallProgressions[0].Conditions[0].Reason != addonv1alpha1.ProgressingReasonUpgradeSucceed {

diff --git a/pkg/addon/controllers/addonprogressing/controller_test.go b/pkg/addon/controllers/addonprogressing/controller_test.go
@@ -25,11 +25,6 @@ import (
 	testingcommon "open-cluster-management.io/ocm/pkg/common/testing"
 )
 
-func newClusterManagementOwner(name string) metav1.OwnerReference {
-	clusterManagementAddon := addontesting.NewClusterManagementAddon(name, "testcrd", "testcr").Build()
-	return *metav1.NewControllerRef(clusterManagementAddon, addonapiv1alpha1.GroupVersion.WithKind("ClusterManagementAddOn"))
-}
-
 func TestReconcile(t *testing.T) {
 	cases := []struct {
 		name                   string

diff --git a/pkg/addon/templateagent/registration_test.go b/pkg/addon/templateagent/registration_test.go
@@ -9,6 +9,7 @@ import (
 	"time"
 
 	certificatesv1 "k8s.io/api/certificates/v1"
+	certificates "k8s.io/api/certificates/v1beta1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -66,7 +67,7 @@ func TestTemplateCSRConfigurationsFunc(t *testing.T) {
 			addon: NewFakeTemplateManagedClusterAddon("addon1", "cluster1", "template1", "fakehash"),
 			expectedConfigs: []addonapiv1alpha1.RegistrationConfig{
 				{
-					SignerName: "kubernetes.io/kube-apiserver-client",
+					SignerName: certificates.KubeAPIServerClientSignerName,
 					Subject: addonapiv1alpha1.Subject{
 						User: "system:open-cluster-management:cluster:cluster1:addon:addon1:agent:agent1",
 
@@ -188,7 +189,7 @@ func TestTemplateCSRApproveCheckFunc(t *testing.T) {
 					Name: "csr1",
 				},
 				Spec: certificatesv1.CertificateSigningRequestSpec{
-					SignerName: "kubernetes.io/kube-apiserver-client",
+					SignerName: certificates.KubeAPIServerClientSignerName,
 				},
 			},
 			expectedApprove: false, // fake csr data
@@ -288,7 +289,7 @@ func TestTemplateCSRSignFunc(t *testing.T) {
 					Name: "csr1",
 				},
 				Spec: certificatesv1.CertificateSigningRequestSpec{
-					SignerName: "kubernetes.io/kube-apiserver-client",
+					SignerName: certificates.KubeAPIServerClientSignerName,
 					Username:   "system:open-cluster-management:cluster1:adcde",
 				},
 			},
@@ -356,7 +357,7 @@ func NewFakeManagedCluster(name string) *clusterv1.ManagedCluster {
 	return &clusterv1.ManagedCluster{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "ManagedCluster",
-			APIVersion: clusterv1.SchemeGroupVersion.String(),
+			APIVersion: clusterv1.GroupVersion.String(),
 		},
 		ObjectMeta: metav1.ObjectMeta{
 			Name: name,

diff --git a/pkg/common/apply/rbac.go b/pkg/common/apply/rbac.go
@@ -43,7 +43,7 @@ func (m *PermissionApplier) Apply(
 	recorder events.Recorder,
 	manifests resourceapply.AssetFunc,
 	files ...string) []resourceapply.ApplyResult {
-	ret := []resourceapply.ApplyResult{}
+	var ret []resourceapply.ApplyResult
 	for _, file := range files {
 		result := resourceapply.ApplyResult{File: file}
 		objBytes, err := manifests(file)
@@ -73,7 +73,7 @@ func (m *PermissionApplier) Apply(
 			result.Result, result.Changed, result.Error = Apply[*rbacv1.RoleBinding](
 				ctx, m.roleBindingLister.RoleBindings(t.Namespace), m.client.RbacV1().RoleBindings(t.Namespace), compareRoleBinding, t, recorder)
 		default:
-			result.Error = fmt.Errorf("object type is not correct.")
+			result.Error = fmt.Errorf("object type is not correct")
 		}
 	}
 	return ret

diff --git a/pkg/common/apply/rbac_test.go b/pkg/common/apply/rbac_test.go
@@ -65,7 +65,7 @@ rules:
 			},
 		},
 		{
-			name: "comapre and no update clusterrole",
+			name: "compare and no update clusterrole",
 			existingManifest: `
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole
@@ -220,7 +220,7 @@ rules:
 			},
 		},
 		{
-			name: "comapre and no update clusterrole",
+			name: "compare and no update clusterrole",
 			existingManifest: `
 apiVersion: rbac.authorization.k8s.io/v1
 kind: Role
@@ -351,13 +351,16 @@ subjects:
 				informerFactory = informers.NewSharedInformerFactory(kubeClient, 3*time.Minute)
 				switch t := o.(type) {
 				case *rbacv1.ClusterRole:
-					informerFactory.Rbac().V1().ClusterRoles().Informer().GetStore().Add(t)
+					err = informerFactory.Rbac().V1().ClusterRoles().Informer().GetStore().Add(t)
 				case *rbacv1.ClusterRoleBinding:
-					informerFactory.Rbac().V1().ClusterRoleBindings().Informer().GetStore().Add(t)
+					err = informerFactory.Rbac().V1().ClusterRoleBindings().Informer().GetStore().Add(t)
 				case *rbacv1.Role:
-					informerFactory.Rbac().V1().Roles().Informer().GetStore().Add(t)
+					err = informerFactory.Rbac().V1().Roles().Informer().GetStore().Add(t)
 				case *rbacv1.RoleBinding:
-					informerFactory.Rbac().V1().RoleBindings().Informer().GetStore().Add(t)
+					err = informerFactory.Rbac().V1().RoleBindings().Informer().GetStore().Add(t)
+				}
+				if err != nil {
+					t.Fatal(err)
 				}
 			} else {
 				kubeClient = kubefake.NewSimpleClientset()

diff --git a/pkg/common/options/agent.go b/pkg/common/options/agent.go
@@ -65,7 +65,7 @@ func (o *AgentOptions) AddFlags(flags *pflag.FlagSet) {
 	flags.StringVar(&o.AgentID, "agent-id", o.AgentID, "ID of the agent")
 }
 
-// spokeKubeConfig builds kubeconfig for the spoke/managed cluster
+// SpokeKubeConfig builds kubeconfig for the spoke/managed cluster
 func (o *AgentOptions) SpokeKubeConfig(managedRestConfig *rest.Config) (*rest.Config, error) {
 	if o.SpokeKubeconfigFile == "" {
 		managedRestConfig.QPS = o.CommoOpts.QPS

diff --git a/pkg/common/options/options.go b/pkg/common/options/options.go
@@ -14,7 +14,7 @@ type Options struct {
 	QPS       float32
 }
 
-// NewAgentOptions returns the flags with default value set
+// NewOptions returns the flags with default value set
 func NewOptions() *Options {
 	opts := &Options{
 		QPS:   50,

diff --git a/pkg/common/options/options_test.go b/pkg/common/options/options_test.go
@@ -58,35 +58,42 @@ func TestComplete(t *testing.T) {
 		{
 			name:        "override cluster name in cert with specified value",
 			clusterName: "cluster1",
-			secret: testinghelpers.NewHubKubeconfigSecret(componentNamespace, "hub-kubeconfig-secret", "", testinghelpers.NewTestCert("system:open-cluster-management:cluster2:agent2", 60*time.Second), map[string][]byte{
-				"kubeconfig":   testinghelpers.NewKubeconfig(nil, nil),
-				"cluster-name": []byte("cluster3"),
-				"agent-name":   []byte("agent3"),
-			}),
+			secret: testinghelpers.NewHubKubeconfigSecret(
+				componentNamespace, "hub-kubeconfig-secret", "",
+				testinghelpers.NewTestCert("system:open-cluster-management:cluster2:agent2", 60*time.Second), map[string][]byte{
+					"kubeconfig":   testinghelpers.NewKubeconfig(nil, nil),
+					"cluster-name": []byte("cluster3"),
+					"agent-name":   []byte("agent3"),
+				}),
 			expectedClusterName: "cluster1",
 			expectedAgentName:   "agent2",
 		},
 		{
 			name: "take cluster/agent name from secret",
-			secret: testinghelpers.NewHubKubeconfigSecret(componentNamespace, "hub-kubeconfig-secret", "", nil, map[string][]byte{
-				"cluster-name": []byte("cluster1"),
-				"agent-name":   []byte("agent1"),
-			}),
+			secret: testinghelpers.NewHubKubeconfigSecret(
+				componentNamespace, "hub-kubeconfig-secret", "", nil, map[string][]byte{
+					"cluster-name": []byte("cluster1"),
+					"agent-name":   []byte("agent1"),
+				}),
 			expectedClusterName: "cluster1",
 			expectedAgentName:   "agent1",
 		},
 		{
-			name:                "take cluster/agent name from cert",
-			secret:              testinghelpers.NewHubKubeconfigSecret(componentNamespace, "hub-kubeconfig-secret", "", testinghelpers.NewTestCert("system:open-cluster-management:cluster1:agent1", 60*time.Second), map[string][]byte{}),
+			name: "take cluster/agent name from cert",
+			secret: testinghelpers.NewHubKubeconfigSecret(
+				componentNamespace, "hub-kubeconfig-secret", "",
+				testinghelpers.NewTestCert("system:open-cluster-management:cluster1:agent1", 60*time.Second), map[string][]byte{}),
 			expectedClusterName: "cluster1",
 			expectedAgentName:   "agent1",
 		},
 		{
 			name: "override cluster name in secret with value from cert",
-			secret: testinghelpers.NewHubKubeconfigSecret(componentNamespace, "hub-kubeconfig-secret", "", testinghelpers.NewTestCert("system:open-cluster-management:cluster1:agent1", 60*time.Second), map[string][]byte{
-				"cluster-name": []byte("cluster2"),
-				"agent-name":   []byte("agent2"),
-			}),
+			secret: testinghelpers.NewHubKubeconfigSecret(
+				componentNamespace, "hub-kubeconfig-secret", "",
+				testinghelpers.NewTestCert("system:open-cluster-management:cluster1:agent1", 60*time.Second), map[string][]byte{
+					"cluster-name": []byte("cluster2"),
+					"agent-name":   []byte("agent2"),
+				}),
 			expectedClusterName: "cluster1",
 			expectedAgentName:   "agent1",
 		},
@@ -115,6 +122,9 @@ func TestComplete(t *testing.T) {
 			err = registration.DumpSecret(
 				kubeClient.CoreV1(), componentNamespace, "hub-kubeconfig-secret",
 				options.HubKubeconfigDir, context.TODO(), eventstesting.NewTestingEventRecorder(t))
+			if err != nil {
+				t.Error(err)
+			}
 
 			if err := options.Complete(); err != nil {
 				t.Errorf("unexpected error: %v", err)

diff --git a/pkg/common/patcher/patcher.go b/pkg/common/patcher/patcher.go
@@ -16,7 +16,7 @@ import (
 	"k8s.io/klog/v2"
 )
 
-// Patcher is just the Patch API with a generic to keep use sites type safe.
+// PatchClient is just the Patch API with a generic to keep use sites type safe.
 // This is inspired by the commiter code in https://github.com/kcp-dev/kcp/blob/main/pkg/reconciler/committer/committer.go
 type PatchClient[R runtime.Object] interface {
 	Patch(ctx context.Context, name string, pt types.PatchType, data []byte, opts metav1.PatchOptions, subresources ...string) (R, error)
@@ -28,6 +28,7 @@ type Patcher[R runtime.Object, Sp any, St any] interface {
 	PatchStatus(context.Context, R, St, St) (bool, error)
 	PatchSpec(context.Context, R, Sp, Sp) (bool, error)
 	PatchLabelAnnotations(context.Context, R, metav1.ObjectMeta, metav1.ObjectMeta) (bool, error)
+	WithOptions(options PatchOptions) Patcher[R, Sp, St]
 }
 
 type PatchOptions struct {
@@ -47,14 +48,14 @@ type patcher[R runtime.Object, Sp any, St any] struct {
 	opts   PatchOptions
 }
 
-func NewPatcher[R runtime.Object, Sp any, St any](client PatchClient[R]) *patcher[R, Sp, St] {
+func NewPatcher[R runtime.Object, Sp any, St any](client PatchClient[R]) Patcher[R, Sp, St] {
 	p := &patcher[R, Sp, St]{
 		client: client,
 	}
 	return p
 }
 
-func (p *patcher[R, Sp, St]) WithOptions(options PatchOptions) *patcher[R, Sp, St] {
+func (p *patcher[R, Sp, St]) WithOptions(options PatchOptions) Patcher[R, Sp, St] {
 	p.opts = options
 	return p
 }
@@ -66,7 +67,7 @@ func (p *patcher[R, Sp, St]) AddFinalizer(ctx context.Context, object R, finaliz
 	}
 
 	existingFinalizers := accessor.GetFinalizers()
-	finalizersToAdd := []string{}
+	var finalizersToAdd []string
 	for _, finalizer := range finalizers {
 		hasFinalizer := false
 		for i := range existingFinalizers {
@@ -120,7 +121,7 @@ func (p *patcher[R, Sp, St]) RemoveFinalizer(ctx context.Context, object R, fina
 		return err
 	}
 
-	copiedFinalizers := []string{}
+	var copiedFinalizers []string
 	existingFinalizers := accessor.GetFinalizers()
 	for i := range existingFinalizers {
 		matchFinalizer := false

diff --git a/pkg/common/patcher/patcher_test.go b/pkg/common/patcher/patcher_test.go
@@ -328,7 +328,9 @@ func TestPatchLabelAnnotations(t *testing.T) {
 				if err != nil {
 					t.Fatal(err)
 				}
-				if !equality.Semantic.DeepEqual(labelPatch["metadata"], map[string]interface{}{"uid": "", "resourceVersion": "", "labels": map[string]interface{}{"key1": nil}}) {
+				if !equality.Semantic.DeepEqual(
+					labelPatch["metadata"],
+					map[string]interface{}{"uid": "", "resourceVersion": "", "labels": map[string]interface{}{"key1": nil}}) {
 					t.Errorf("not patched correctly got %v", labelPatch)
 				}
 			},

diff --git a/pkg/operator/certrotation/cabundle.go b/pkg/operator/certrotation/cabundle.go
@@ -71,7 +71,7 @@ func manageCABundleConfigMap(caBundleConfigMap *corev1.ConfigMap, currentSigner
 		caBundleConfigMap.Data = map[string]string{}
 	}
 
-	certificates := []*x509.Certificate{}
+	var certificates []*x509.Certificate
 	caBundle := caBundleConfigMap.Data["ca-bundle.crt"]
 	if len(caBundle) > 0 {
 		var err error
@@ -83,7 +83,7 @@ func manageCABundleConfigMap(caBundleConfigMap *corev1.ConfigMap, currentSigner
 	certificates = append([]*x509.Certificate{currentSigner}, certificates...)
 	certificates = crypto.FilterExpiredCerts(certificates...)
 
-	finalCertificates := []*x509.Certificate{}
+	var finalCertificates []*x509.Certificate
 	// now check for duplicates. n^2, but super simple
 	for i := range certificates {
 		found := false

diff --git a/pkg/operator/certrotation/cabundle_test.go b/pkg/operator/certrotation/cabundle_test.go
@@ -85,7 +85,7 @@ func TestManageCABundleConfigMap(t *testing.T) {
 				}
 
 				if !reflect.DeepEqual(c.signerCert, caCerts[0]) {
-					t.Fatalf("Current signer cert should be put at the begining")
+					t.Fatalf("Current signer cert should be put at the beginning")
 				}
 			}
 		})

diff --git a/pkg/operator/certrotation/target_test.go b/pkg/operator/certrotation/target_test.go
@@ -167,9 +167,9 @@ func TestNeedNewTargetCertKeyPair(t *testing.T) {
 
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
-			caBundleCerts := []*x509.Certificate{}
+			var caBundleCerts []*x509.Certificate
 			if len(c.caBundle) > 0 {
-				caBundleCerts, err = cert.ParseCertsPEM([]byte(c.caBundle))
+				caBundleCerts, err = cert.ParseCertsPEM(c.caBundle)
 				if err != nil {
 					t.Fatalf("Expected no error, but got: %v", err)
 				}

diff --git a/pkg/operator/helpers/helpers.go b/pkg/operator/helpers/helpers.go
@@ -264,9 +264,9 @@ func ApplyDirectly(
 	cache resourceapply.ResourceCache,
 	manifests resourceapply.AssetFunc,
 	files ...string) []resourceapply.ApplyResult {
-	ret := []resourceapply.ApplyResult{}
+	var ret []resourceapply.ApplyResult
 
-	genericApplyFiles := []string{}
+	var genericApplyFiles []string
 	for _, file := range files {
 		result := resourceapply.ApplyResult{File: file}
 		objBytes, err := manifests(file)