chore: enable more rules from go-critic by default

.golangci.yaml

@@ -18,9 +18,17 @@ linters:
     gocritic:
       disabled-checks:
         - appendAssign
-        - assignOp
+        - badRegexp
+        - builtinShadow
+        - commentedOutCode
         - exitAfterDefer
-        - typeSwitchVar
+        - hugeParam
+        - importShadow
+        - paramTypeCombine
+        - rangeValCopy
+        - unnamedResult
+        - whyNoLint
+      enable-all: true
     importas:
       alias:
         - pkg: k8s.io/api/apps/v1

pkg/cache/cluster.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"runtime/debug"
-	"strings"
 	"sync"
 	"time"
 
@@ -1140,7 +1139,7 @@ func buildGraph(nsNodes map[kube.ResourceKey]*Resource) map[kube.ResourceKey]map
 						// It is ok to pick any object, but we need to make sure we pick the same child after every refresh.
 						key1 := r.ResourceKey()
 						key2 := childNode.ResourceKey()
-						if strings.Compare(key1.String(), key2.String()) > 0 {
+						if key1.String() > key2.String() {
 							graph[uidNode.ResourceKey()][childNode.Ref.UID] = childNode
 						}
 					}

pkg/cache/cluster_test.go

@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"sort"
-	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -681,7 +680,7 @@ func TestProcessNewChildEvent(t *testing.T) {
 
 	rsChildren := getChildren(cluster, mustToUnstructured(testRS()))
 	sort.Slice(rsChildren, func(i, j int) bool {
-		return strings.Compare(rsChildren[i].Ref.Name, rsChildren[j].Ref.Name) < 0
+		return rsChildren[i].Ref.Name < rsChildren[j].Ref.Name
 	})
 	assert.Equal(t, []*Resource{{
 		Ref: corev1.ObjectReference{

pkg/diff/diff.go

@@ -473,7 +473,7 @@ func normalizeTypedValue(tv *typed.TypedValue) ([]byte, error) {
 
 func buildDiffResult(predictedBytes []byte, liveBytes []byte) *DiffResult {
 	return &DiffResult{
-		Modified:       string(liveBytes) != string(predictedBytes),
+		Modified:       !bytes.Equal(liveBytes, predictedBytes),
 		NormalizedLive: liveBytes,
 		PredictedLive:  predictedBytes,
 	}
@@ -1106,7 +1106,7 @@ func hide(target, live, liveLastAppliedAnnotation *unstructured.Unstructured, ke
 			replacement, ok := valToReplacement[val]
 			if !ok {
 				replacement = nextReplacement
-				nextReplacement = nextReplacement + "++++"
+				nextReplacement += "++++"
 				valToReplacement[val] = replacement
 			}
 			data[k] = replacement

pkg/sync/sync_context.go

@@ -57,6 +57,57 @@ type SyncContext interface {
 	GetState() (common.OperationPhase, string, []common.ResourceSyncResult)
 }
 
+type syncContext struct {
+	healthOverride      health.HealthOverride
+	permissionValidator common.PermissionValidator
+	resources           map[kubeutil.ResourceKey]reconciledResource
+	hooks               []*unstructured.Unstructured
+	config              *rest.Config
+	rawConfig           *rest.Config
+	dynamicIf           dynamic.Interface
+	disco               discovery.DiscoveryInterface
+	extensionsclientset *clientset.Clientset
+	kubectl             kubeutil.Kubectl
+	resourceOps         kubeutil.ResourceOperations
+	namespace           string
+
+	dryRun                          bool
+	skipDryRunOnMissingResource     bool
+	force                           bool
+	validate                        bool
+	skipHooks                       bool
+	resourcesFilter                 func(key kubeutil.ResourceKey, target *unstructured.Unstructured, live *unstructured.Unstructured) bool
+	prune                           bool
+	replace                         bool
+	serverSideApply                 bool
+	serverSideApplyManager          string
+	pruneLast                       bool
+	prunePropagationPolicy          *metav1.DeletionPropagation
+	pruneConfirmed                  bool
+	clientSideApplyMigrationManager string
+	enableClientSideApplyMigration  bool
+
+	syncRes   map[string]common.ResourceSyncResult
+	startedAt time.Time
+	revision  string
+	phase     common.OperationPhase
+	message   string
+
+	log logr.Logger
+	// lock to protect concurrent updates of the result list
+	lock sync.Mutex
+
+	// syncNamespace is a function that will determine if the managed
+	// namespace should be synced
+	syncNamespace func(*unstructured.Unstructured, *unstructured.Unstructured) (bool, error)
+
+	syncWaveHook common.SyncWaveHook
+
+	applyOutOfSyncOnly bool
+	// stores whether the resource is modified or not
+	modificationResult map[kubeutil.ResourceKey]bool
+}
+
 // SyncOpt is a callback that update sync operation settings
 type SyncOpt func(ctx *syncContext)
 
@@ -340,57 +391,6 @@ func (sc *syncContext) getOperationPhase(obj *unstructured.Unstructured) (common
 	return phase, message, nil
 }
 
-type syncContext struct {
-	healthOverride      health.HealthOverride
-	permissionValidator common.PermissionValidator
-	resources           map[kubeutil.ResourceKey]reconciledResource
-	hooks               []*unstructured.Unstructured
-	config              *rest.Config
-	rawConfig           *rest.Config
-	dynamicIf           dynamic.Interface
-	disco               discovery.DiscoveryInterface
-	extensionsclientset *clientset.Clientset
-	kubectl             kubeutil.Kubectl
-	resourceOps         kubeutil.ResourceOperations
-	namespace           string
-
-	dryRun                          bool
-	skipDryRunOnMissingResource     bool
-	force                           bool
-	validate                        bool
-	skipHooks                       bool
-	resourcesFilter                 func(key kubeutil.ResourceKey, target *unstructured.Unstructured, live *unstructured.Unstructured) bool
-	prune                           bool
-	replace                         bool
-	serverSideApply                 bool
-	serverSideApplyManager          string
-	pruneLast                       bool
-	prunePropagationPolicy          *metav1.DeletionPropagation
-	pruneConfirmed                  bool
-	clientSideApplyMigrationManager string
-	enableClientSideApplyMigration  bool
-
-	syncRes   map[string]common.ResourceSyncResult
-	startedAt time.Time
-	revision  string
-	phase     common.OperationPhase
-	message   string
-
-	log logr.Logger
-	// lock to protect concurrent updates of the result list
-	lock sync.Mutex
-
-	// syncNamespace is a function that will determine if the managed
-	// namespace should be synced
-	syncNamespace func(*unstructured.Unstructured, *unstructured.Unstructured) (bool, error)
-
-	syncWaveHook common.SyncWaveHook
-
-	applyOutOfSyncOnly bool
-	// stores whether the resource is modified or not
-	modificationResult map[kubeutil.ResourceKey]bool
-}
-
 func (sc *syncContext) setRunningPhase(tasks []*syncTask, isPendingDeletion bool) {
 	if len(tasks) > 0 {
 		firstTask := tasks[0]
@@ -939,7 +939,7 @@ func (sc *syncContext) getSyncTasks() (_ syncTasks, successful bool) {
 			}
 		}
 	}
-	syncPhaseLastWave = syncPhaseLastWave + 1
+	syncPhaseLastWave++
 
 	for _, task := range tasks {
 		if task.isPrune() &&

pkg/sync/sync_context_test.go

@@ -1827,8 +1827,11 @@ func TestSetOperationFailedDuplicatedMessages(t *testing.T) {
 	sc.log = textlogger.NewLogger(textlogger.NewConfig()).WithValues("application", "fake-app")
 
 	tasks := make([]*syncTask, 0)
-	tasks = append(tasks, &syncTask{message: "namespace not found"})
-	tasks = append(tasks, &syncTask{message: "namespace not found"})
+	tasks = append(
+		tasks,
+		&syncTask{message: "namespace not found"},
+		&syncTask{message: "namespace not found"},
+	)
 
 	sc.setOperationFailed(nil, tasks, "one or more objects failed to apply")
 

pkg/utils/kube/resource_ops.go

@@ -140,10 +140,10 @@ func (k *kubectlResourceOperations) runResourceCommand(ctx context.Context, obj
 	if err != nil {
 		return "", errors.New(cleanKubectlOutput(err.Error()))
 	}
-	if buf := strings.TrimSpace(ioStreams.Out.(*bytes.Buffer).String()); len(buf) > 0 {
+	if buf := strings.TrimSpace(ioStreams.Out.(*bytes.Buffer).String()); buf != "" {
 		out = append(out, buf)
 	}
-	if buf := strings.TrimSpace(ioStreams.ErrOut.(*bytes.Buffer).String()); len(buf) > 0 {
+	if buf := strings.TrimSpace(ioStreams.ErrOut.(*bytes.Buffer).String()); buf != "" {
 		out = append(out, buf)
 	}
 	return strings.Join(out, ". "), nil
@@ -608,10 +608,10 @@ func (k *kubectlResourceOperations) authReconcile(ctx context.Context, obj *unst
 	}
 
 	var out []string
-	if buf := strings.TrimSpace(ioStreams.Out.(*bytes.Buffer).String()); len(buf) > 0 {
+	if buf := strings.TrimSpace(ioStreams.Out.(*bytes.Buffer).String()); buf != "" {
 		out = append(out, buf)
 	}
-	if buf := strings.TrimSpace(ioStreams.ErrOut.(*bytes.Buffer).String()); len(buf) > 0 {
+	if buf := strings.TrimSpace(ioStreams.ErrOut.(*bytes.Buffer).String()); buf != "" {
 		out = append(out, buf)
 	}
 	return strings.Join(out, ". "), nil

pkg/utils/kube/uniqueprotomodels.go

@@ -104,7 +104,7 @@ func newUniqueModels(models proto.Models) (proto.Models, []schema.GroupVersionKi
 
 			// Add GVKs to the map, so we can detect a duplicate GVK later.
 			for _, gvk := range gvkList {
-				if len(gvk.Kind) > 0 {
+				if gvk.Kind != "" {
 					gvks[gvk] = modelName
 				}
 			}
@@ -124,7 +124,7 @@ func modelGvkWasAlreadyProcessed(model proto.Schema, gvks map[schema.GroupVersio
 	for _, gvk := range gvkList {
 		// The kind length check is copied from managedfields.NewGVKParser. It's unclear what edge case it's handling,
 		// but the behavior of this function should match NewGVKParser.
-		if len(gvk.Kind) > 0 {
+		if gvk.Kind != "" {
 			_, ok := gvks[gvk]
 			if ok {
 				// This is the only condition under which NewGVKParser would return a duplicate GVK error.

pkg/utils/text/text.go

@@ -2,7 +2,7 @@ package text
 
 func FirstNonEmpty(args ...string) string {
 	for _, value := range args {
-		if len(value) > 0 {
+		if value != "" {
 			return value
 		}
 	}
@@ -11,7 +11,7 @@ func FirstNonEmpty(args ...string) string {
 
 // WithDefault return defaultValue when val is blank
 func WithDefault(val string, defaultValue string) string {
-	if len(val) == 0 {
+	if val == "" {
 		return defaultValue
 	}
 	return val