Enable gocritic and gocyclo linters (#2236)

* Enable gocyclo linter

* Enable gocritic linter

* Fix typo for linters-settings, fix gocyclo lint failures

* Update new lint misses, add TODO for lint disables

* Apply linter to new changes

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

.golangci.yml

@@ -12,6 +12,8 @@ linters:
     - whitespace
     - gocognit
     - unparam
+    - gocyclo
+    - gocritic
 
 run:
   timeout: 10m                          # golangci-lint run's timeout.
@@ -29,3 +31,7 @@ issues:
     - text: "`ctx` is unused"           # Context might not be in use in places, but for consistency, we pass it.
       linters:
         - unparam
+
+linters-settings:
+  gocyclo:
+    min-complexity: 20

cmd/kanctl/main.go

@@ -21,7 +21,7 @@ import (
 
 func init() {
 	// We silence all non-fatal log messages.
-	//logrus.SetLevel(logrus.ErrorLevel)
+	// logrus.SetLevel(logrus.ErrorLevel)
 }
 
 func main() {

pkg/blockstorage/awsefs/awsefs.go

@@ -565,7 +565,7 @@ func (e *Efs) SnapshotsListWLimit(ctx context.Context, tags map[string]string, l
 	req := &backup.ListRecoveryPointsByBackupVaultInput{}
 	req.SetBackupVaultName(e.backupVaultName)
 	req.SetMaxResults(limit)
-	resp, err := e.ListRecoveryPointsByBackupVaultWithContext(ctx, req) //backup API
+	resp, err := e.ListRecoveryPointsByBackupVaultWithContext(ctx, req) // backup API
 	if err != nil {
 		return nil, errors.Wrap(err, "Failed to list recovery points by vault")
 	}

pkg/blockstorage/azure/azuredisk.go

@@ -199,47 +199,20 @@ func (s *AdStorage) SnapshotCopyWithArgs(ctx context.Context, from blockstorage.
 	blob := container.GetBlobReference(blobName)
 	defer deleteBlob(blob, blobName)
 
-	var copyOptions *storage.CopyOptions
-	if t, ok := ctx.Deadline(); ok {
-		time := time.Until(t).Seconds()
-		if time <= 0 {
-			return nil, errors.New("Context deadline exceeded, cannot copy snapshot")
-		}
-		copyOptions = &storage.CopyOptions{
-			Timeout: uint(time),
-		}
+	copyOptions, err := getCopyOptions(ctx)
+	if err != nil {
+		return nil, err
 	}
 	err = blob.Copy(*accessURI.AccessSAS, copyOptions)
 	if err != nil {
 		return nil, errors.Wrapf(err, "Failed to copy disk to blob")
 	}
-	blobURI := blob.GetURL()
-
 	snapId, err := uuid.NewV1()
 	if err != nil {
 		return nil, errors.Wrap(err, "Failed to create UUID")
 	}
 	snapName := fmt.Sprintf(snapshotNameFmt, snapId.String())
-	var tags = make(map[string]string)
-	for _, tag := range from.Volume.Tags {
-		if _, found := tags[tag.Key]; !found {
-			tags[tag.Key] = tag.Value
-		}
-	}
-	tags = blockstorage.SanitizeTags(ktags.GetTags(tags))
-
-	createSnap := azcompute.Snapshot{
-		Name:     azto.StringPtr(snapName),
-		Location: azto.StringPtr(to.Region),
-		Tags:     *azto.StringMapPtr(tags),
-		SnapshotProperties: &azcompute.SnapshotProperties{
-			CreationData: &azcompute.CreationData{
-				CreateOption:     azcompute.Import,
-				StorageAccountID: azto.StringPtr(storageAccountID),
-				SourceURI:        azto.StringPtr(blobURI),
-			},
-		},
-	}
+	createSnap := getSnapshotObject(blob, from, to, snapName, storageAccountID)
 
 	migrateResourceGroup := s.azCli.ResourceGroup
 	if val, ok := args[blockstorage.AzureMigrateResourceGroup]; ok && val != "" {
@@ -266,14 +239,60 @@ func (s *AdStorage) SnapshotCopyWithArgs(ctx context.Context, from blockstorage.
 	return snap, nil
 }
 
+func getCopyOptions(ctx context.Context) (*storage.CopyOptions, error) {
+	var copyOptions *storage.CopyOptions
+	if t, ok := ctx.Deadline(); ok {
+		time := time.Until(t).Seconds()
+		if time <= 0 {
+			return nil, errors.New("Context deadline exceeded, cannot copy snapshot")
+		}
+		copyOptions = &storage.CopyOptions{
+			Timeout: uint(time),
+		}
+	}
+	return copyOptions, nil
+}
+
+func getSnapshotObject(
+	blob *storage.Blob,
+	from,
+	to blockstorage.Snapshot,
+	snapName,
+	storageAccountID string,
+) azcompute.Snapshot {
+	blobURI := blob.GetURL()
+
+	var tags = make(map[string]string)
+	for _, tag := range from.Volume.Tags {
+		if _, found := tags[tag.Key]; !found {
+			tags[tag.Key] = tag.Value
+		}
+	}
+	tags = blockstorage.SanitizeTags(ktags.GetTags(tags))
+
+	createSnap := azcompute.Snapshot{
+		Name:     azto.StringPtr(snapName),
+		Location: azto.StringPtr(to.Region),
+		Tags:     *azto.StringMapPtr(tags),
+		SnapshotProperties: &azcompute.SnapshotProperties{
+			CreationData: &azcompute.CreationData{
+				CreateOption:     azcompute.Import,
+				StorageAccountID: azto.StringPtr(storageAccountID),
+				SourceURI:        azto.StringPtr(blobURI),
+			},
+		},
+	}
+	return createSnap
+}
+
 func isMigrateStorageAccountorKey(migrateStorageAccount, migrateStorageKey string) bool {
 	return migrateStorageAccount == "" || migrateStorageKey == ""
 }
 
-func (s *AdStorage) revokeAccess(ctx context.Context, rg, name, ID string) {
+func (s *AdStorage) revokeAccess(ctx context.Context, rg, name, id string) {
 	_, err := s.azCli.SnapshotsClient.RevokeAccess(ctx, rg, name)
 	if err != nil {
-		log.Print("Failed to revoke access from snapshot", field.M{"snapshot": ID})
+		log.Print("Failed to revoke access from snapshot", field.M{"snapshot": id})
 	}
 }
 
@@ -554,7 +573,7 @@ func (s *AdStorage) VolumeCreateFromSnapshot(ctx context.Context, snapshot block
 }
 
 func (s *AdStorage) getRegionAndZoneID(ctx context.Context, sourceRegion, volAz string) (string, string, error) {
-	//check if current node region is zoned or not
+	// check if current node region is zoned or not
 	kubeCli, err := kube.NewClient()
 	if err != nil {
 		return "", "", err

pkg/blockstorage/tags/tags.go

@@ -103,14 +103,14 @@ func IsSubset(set map[string]string, subset map[string]string) bool {
 	return true
 }
 
-// Union returns union of A and B as a new map.
-// B's values have priority if a key from A and B collides.
-func Union(A map[string]string, B map[string]string) map[string]string {
+// Union returns union of first and second as a new map.
+// second's values have priority if a key from first and second collides.
+func Union(first map[string]string, second map[string]string) map[string]string {
 	result := make(map[string]string)
-	for k, v := range A {
+	for k, v := range first {
 		result[k] = v
 	}
-	for k, v := range B {
+	for k, v := range second {
 		result[k] = v
 	}
 	return result

pkg/blockstorage/vmware/vmware.go

@@ -811,11 +811,12 @@ func (ge govmomiError) ExtractMessages() []string {
 		case govmomitask.Error:
 			foundHandledErrorType = true
 		default:
-			if soap.IsSoapFault(err) {
+			switch {
+			case soap.IsSoapFault(err):
 				foundHandledErrorType = true
-			} else if soap.IsVimFault(err) {
+			case soap.IsVimFault(err):
 				foundHandledErrorType = true
-			} else {
+			default:
 				err = errors.Unwrap(err)
 			}
 		}

pkg/blockstorage/zone/zone_test.go

@@ -23,7 +23,7 @@ import (
 
 	"github.com/kanisterio/kanister/pkg/kube"
 	. "gopkg.in/check.v1"
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/fake"
@@ -543,14 +543,14 @@ func (s ZoneSuite) TestFromSourceRegionZone(c *C) {
 		outZones []string
 		outErr   error
 	}{
-		{ //success case
+		{ // success case
 			inRegion: "us-west-2",
 			inZones:  []string{"us-west-2a"},
 			inCli:    cli,
 			outZones: []string{"us-west-2a"},
 			outErr:   nil,
 		},
-		{ //success case gce multi region
+		{ // success case gce multi region
 			inRegion: "us-west1",
 			inZones:  []string{"us-west1-a"},
 			inCli:    cligce,

pkg/blueprint/validate/validate.go

@@ -73,9 +73,10 @@ func Do(bp *crv1alpha1.Blueprint, funcVersion string) error {
 func validatePhaseNames(bp *crv1alpha1.Blueprint) error {
 	phasesCount := make(map[string]int)
 	for _, action := range bp.Actions {
-		allPhases := action.Phases
+		allPhases := []crv1alpha1.BlueprintPhase{}
+		allPhases = append(allPhases, action.Phases...)
 		if action.DeferPhase != nil {
-			allPhases = append(action.Phases, *action.DeferPhase)
+			allPhases = append(allPhases, *action.DeferPhase)
 		}
 
 		for _, phase := range allPhases {

pkg/controller/controller.go

@@ -246,11 +246,12 @@ func (c *Controller) onUpdateActionSet(oldAS, newAS *crv1alpha1.ActionSet) error
 		return err
 	}
 	if newAS.Status == nil || newAS.Status.State != crv1alpha1.StateRunning {
-		if newAS.Status == nil {
+		switch {
+		case newAS.Status == nil:
 			log.WithContext(ctx).Print("Updated ActionSet", field.M{"Status": "nil"})
-		} else if newAS.Status.State == crv1alpha1.StateComplete {
+		case newAS.Status.State == crv1alpha1.StateComplete:
 			c.logAndSuccessEvent(ctx, fmt.Sprintf("Updated ActionSet '%s' Status->%s", newAS.Name, newAS.Status.State), "Update Complete", newAS)
-		} else {
+		default:
 			log.WithContext(ctx).Print("Updated ActionSet", field.M{"Status": newAS.Status.State})
 		}
 		return nil

pkg/controller/controller_test.go

@@ -703,15 +703,15 @@ func (s *ControllerSuite) TestRuntimeObjEventLogs(c *C) {
 	msg := "Unit testing event logs"
 	reason := "Test Logs"
 
-	//Create nil ActionSet
+	// Create nil ActionSet
 	var nilAs = (*crv1alpha1.ActionSet)(nil)
 
 	// Create Blueprint
 	bp := testutil.NewTestBlueprint("StatefulSet", testutil.WaitFuncName)
 	bp, err = s.crCli.Blueprints(s.namespace).Create(ctx, bp, metav1.CreateOptions{})
 	c.Assert(err, IsNil)
 
-	//Test the logAndErrorEvent function
+	// Test the logAndErrorEvent function
 	ctx = field.Context(ctx, consts.ActionsetNameKey, as.GetName())
 	config, err := kube.LoadConfig()
 	c.Assert(err, IsNil)
@@ -725,19 +725,19 @@ func (s *ControllerSuite) TestRuntimeObjEventLogs(c *C) {
 	c.Assert(len(events.Items) > 0, Equals, true)
 	c.Assert(events.Items[0].Message, Equals, msg)
 
-	//Testing nil ActionSet error event logging
+	// Testing nil ActionSet error event logging
 	events, err = s.cli.CoreV1().Events(as.Namespace).Search(scheme.Scheme, nilAs)
 	c.Assert(err, NotNil)
 	c.Assert(len(events.Items), Equals, 0)
 
-	//Testing Blueprint error event logging
+	// Testing Blueprint error event logging
 	events, err = s.cli.CoreV1().Events(bp.Namespace).Search(scheme.Scheme, bp)
 	c.Assert(err, IsNil)
 	c.Assert(events, NotNil)
 	c.Assert(len(events.Items) > 0, Equals, true)
 	c.Assert(events.Items[0].Message, Equals, msg)
 
-	//Testing empty Blueprint
+	// Testing empty Blueprint
 	testbp := &crv1alpha1.Blueprint{}
 	ctlr.logAndErrorEvent(ctx, msg, reason, errors.New("Testing Event Logs"), testbp)
 	events, err = s.cli.CoreV1().Events(bp.Namespace).Search(scheme.Scheme, testbp)

pkg/controllers/repositoryserver/repositoryserver_controller_test.go

@@ -233,7 +233,7 @@ func (s *RepoServerControllerSuite) TestRepositoryServerImmutability(c *C) {
 	repoServerCRCreated, err := s.crCli.RepositoryServers(s.repoServerControllerNamespace).Create(ctx, &repoServerCR, metav1.CreateOptions{})
 	c.Assert(err, IsNil)
 
-	//Update the repository server CR's Immutable field.
+	// Update the repository server CR's Immutable field.
 	patch := []patchStringValue{{
 		Op:    "replace",
 		Path:  "/spec/repository/rootPath",
@@ -265,7 +265,7 @@ func (s *RepoServerControllerSuite) TestRepositoryServerStatusIsServerReady(c *C
 	err = s.waitForRepoServerInfoUpdateInCR(repoServerCRCreated.Name)
 	c.Assert(err, IsNil)
 
-	//Get repository server CR with the updated server information
+	// Get repository server CR with the updated server information
 	repoServerCRCreated, err = s.crCli.RepositoryServers(s.repoServerControllerNamespace).Get(ctx, repoServerCRCreated.Name, metav1.GetOptions{})
 	c.Assert(err, IsNil)
 
@@ -312,7 +312,7 @@ func (s *RepoServerControllerSuite) TestCreationOfOwnedResources(c *C) {
 	err = s.waitForRepoServerInfoUpdateInCR(repoServerCRCreated.Name)
 	c.Assert(err, IsNil)
 
-	//Get repository server CR with the updated server information
+	// Get repository server CR with the updated server information
 	repoServerCRCreated, err = s.crCli.RepositoryServers(s.repoServerControllerNamespace).Get(ctx, repoServerCRCreated.Name, metav1.GetOptions{})
 	c.Assert(err, IsNil)
 
@@ -434,7 +434,7 @@ func (s *RepoServerControllerSuite) TestFilestoreLocationVolumeMountOnRepoServer
 	err = s.waitForRepoServerInfoUpdateInCR(repoServerCRCreated.Name)
 	c.Assert(err, IsNil)
 
-	//Get repository server CR with the updated server information
+	// Get repository server CR with the updated server information
 	repoServerCRCreated, err = s.crCli.RepositoryServers(s.repoServerControllerNamespace).Get(ctx, repoServerCRCreated.Name, metav1.GetOptions{})
 	c.Assert(err, IsNil)
 

pkg/datamover/profile_test.go

@@ -64,7 +64,7 @@ func (ps *ProfileSuite) TestLocationOperationsForProfileDataMover(c *C) {
 	err = locationDelete(ps.ctx, p, path)
 	c.Assert(err, IsNil)
 
-	//test deleting dir with multiple artifacts
+	// test deleting dir with multiple artifacts
 	source = bytes.NewBufferString(testContent)
 	err = locationPush(ps.ctx, p, path, source)
 	c.Assert(err, IsNil)

pkg/filter/filter.go

@@ -145,7 +145,7 @@ type ResourceRequirement struct {
 // DeepCopyInto provides explicit deep copy implementation to avoid
 func (r ResourceRequirement) DeepCopyInto(out *ResourceRequirement) {
 	r.LocalObjectReference.DeepCopyInto(&out.LocalObjectReference)
-	(*out).ResourceTypeRequirement = r.ResourceTypeRequirement
+	out.ResourceTypeRequirement = r.ResourceTypeRequirement
 	r.LabelSelector.DeepCopyInto(&out.LabelSelector)
 }
 

pkg/function/export_rds_snapshot_location.go

@@ -286,8 +286,7 @@ func prepareCommand(
 		return nil, "", err
 	}
 
-	switch dbEngine {
-	case PostgrSQLEngine:
+	if dbEngine == PostgrSQLEngine {
 		switch action {
 		case BackupAction:
 			// For backup operation, if database arg is not set, we take backup of all databases

pkg/kanctl/actionset.go

@@ -88,9 +88,7 @@ func newActionSetCmd() *cobra.Command {
 		Use:   "actionset",
 		Short: "Create a new ActionSet or override a <parent> ActionSet",
 		Args:  cobra.ExactArgs(0),
-		RunE: func(c *cobra.Command, args []string) error {
-			return initializeAndPerform(c, args)
-		},
+		RunE:  initializeAndPerform,
 	}
 	cmd.Flags().StringP(sourceFlagName, "f", "", "specify name of the action set")
 
@@ -558,7 +556,7 @@ func parseGenericObjectReference(s string) (crv1alpha1.ObjectReference, error) {
 	}, nil
 }
 
-func parseObjectsFromSelector(selector, kind, sns string, cli kubernetes.Interface, osCli osversioned.Interface, parsed map[string]bool) ([]crv1alpha1.ObjectReference, error) {
+func parseObjectsFromSelector(selector, kind, sns string, cli kubernetes.Interface, osCli osversioned.Interface, parsed map[string]bool) ([]crv1alpha1.ObjectReference, error) { //nolint:gocyclo
 	ctx := context.Background()
 	var objects []crv1alpha1.ObjectReference
 	appendObj := func(kind, namespace, name string) {