Replace golint with golangci-lint (#139)

* Replace golint with golangci-lint

* Fix linting: context should be first argument

* Fix linting: unused-parameter consider removing or renaming it as _

* Fix linting: error-naming: error var should have form errFoo

* Fix linting: should omit nil check

* Fix linting: unnecessary nil check

* Fix linting: argument is overwritten before first use

* Fix linting: ineffectual assignment

* Fix linting: use strings equalFold

* Fix linting: use simple channel

* Fix linting: simplify return bool statement

* Fix linting: exclude deprectation warnings

* Fix .golangci.yaml file path

* Fix packages to lint path

* Add GOPATH

* rm GOPATH

* Complete source path

* Fix golangci-lint call

* Fix changes after rebase

* Update .golangci.yaml

* Rename unused function param to _

.ci/check

@@ -16,47 +16,27 @@ else
 fi
 
 if [[ -z "${SOURCE_PATH}" ]]; then
-  export SOURCE_PATH="$(${READLINK_BIN} -f $(dirname ${0})/..)"
+  export SOURCE_PATH="$(${READLINK_BIN} -f "$(dirname "${0}")"/..)"
 else
   export SOURCE_PATH="$(${READLINK_BIN} -f "${SOURCE_PATH}")"
 fi
 
-# The `go <cmd>` commands requires to see the target repository to be part of a
-# Go workspace. Thus, if we are not yet in a Go workspace, let's create one
-# temporarily by using symbolic links.
-if [[ "${SOURCE_PATH}" != *"src/github.com/gardener/machine-controller-manager-provider-azure" ]]; then
-  SOURCE_SYMLINK_PATH="${SOURCE_PATH}/tmp/src/github.com/gardener/machine-controller-manager-provider-azure"
-  if [[ -d "${SOURCE_PATH}/tmp" ]]; then
-    rm -rf "${SOURCE_PATH}/tmp"
-  fi
-  mkdir -p "${SOURCE_PATH}/tmp/src/github.com/gardener"
-  ln -s "${SOURCE_PATH}" "${SOURCE_SYMLINK_PATH}"
-  cd "${SOURCE_SYMLINK_PATH}"
-
-  export GOPATH="${SOURCE_PATH}/tmp"
-  export GOBIN="${SOURCE_PATH}/tmp/bin"
-  export PATH="${GOBIN}:${PATH}"
-fi
+export GOBIN="${SOURCE_PATH}/tmp/bin"
+export PATH="${GOBIN}:${PATH}"
 
-# Install Golint (linting tool).
-if ! which golint 1>/dev/null; then
-  echo -n "Installing golint... "
-  GO111MODULE=off go get -u golang.org/x/lint/golint
-  echo "done."
+# Install golangci-lint (linting tool).
+if [[ -z "${GOLANGCI_LINT_VERSION}" ]]; then
+  export GOLANGCI_LINT_VERSION=v1.57.1
 fi
+echo "Fetching golangci-lint tool"
+go install github.com/golangci/golangci-lint/cmd/golangci-lint@"${GOLANGCI_LINT_VERSION}"
+echo "Successfully fetched golangci-lint"
+golangci-lint version
 
 ###############################################################################
 PACKAGES="$(go list -e ./... | grep -vE '/tmp/')"
 LINT_FOLDERS="$(echo ${PACKAGES} | sed "s|github.com/gardener/machine-controller-manager-provider-azure|.|g")"
 
-# Execute static code checks.
-go vet ${PACKAGES}
-
-# Execute automatic code formatting directive.
-go fmt ${PACKAGES}
-
-# Execute lint checks.
-for package in ${LINT_FOLDERS}; do
-    golint -set_exit_status $(find $package -maxdepth 1 -name "*.go" | grep -vE 'zz_generated|_test.go')
-done
-echo "Check script has passed successfully"
+echo "Executing golangci-lint"
+# golangci-lint can't be run from outside the directory
+(cd ${SOURCE_PATH} && golangci-lint run -c .golangci.yaml --timeout 10m)

.golangci.yaml

@@ -0,0 +1,49 @@
+run:
+  concurrency: 4
+  timeout: 10m
+
+linters:
+  disable:
+    - unused
+  enable:
+    - revive
+    - loggercheck
+
+issues:
+  exclude-use-default: false
+  exclude:
+    # errcheck: Almost all programs ignore errors on these functions and in most cases it's ok
+    - Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*printf?|os\.(Un)?Setenv). is not checked
+    # revive:
+    - var-naming # ((var|const|struct field|func) .* should be .*
+    - dot-imports # should not use dot imports
+    - package-comments # package comment should be of the form
+    - unexported-return # exported func .* returns unexported type .*, which can be annoying to use
+    - indent-error-flow # if block ends with a return statement, so drop this else and outdent its block
+    - "exported: (type|func) name will be used as .* by other packages, and that stutters;"
+    # typecheck:
+    - "undeclared name: `.*`"
+    - "\".*\" imported but not used"
+    # allow non-capitalized messages if they start with technical terms
+    - "structured logging message should be capitalized: \"garden(er-apiserver|er-controller-manager|er-admission-controller|er-operator|er-resource-manager|let)"
+  exclude-rules:
+    - linters:
+        - staticcheck
+      text: "SA1019:" # Excludes messages where deprecated variables are used
+
+  exclude-files:
+    - "zz_generated\\..*\\.go$"
+
+linters-settings:
+  revive:
+    rules:
+      - name: duplicated-imports
+      - name: unused-parameter
+      - name: unreachable-code
+      - name: context-as-argument
+      - name: early-return
+      - name: exported
+  loggercheck:
+    no-printf-like: true
+    logr: true
+    zap: true

Makefile

@@ -37,7 +37,7 @@ rename-project:
 
 .PHONY: start
 start:
-	@GO111MODULE=on go run \
+	@go run \
 			cmd/machine-controller/main.go \
 			--control-kubeconfig=$(CONTROL_KUBECONFIG) \
 			--target-kubeconfig=$(TARGET_KUBECONFIG) \
@@ -65,11 +65,11 @@ check:
 
 .PHONY: tidy
 tidy:
-	@env GO111MODULE=on go mod tidy -v
+	@go mod tidy -v
 
 .PHONY: update-dependencies
 update-dependencies:
-	@env GO111MODULE=on go get -u
+	@go get -u ./...
 
 #########################################
 # Rules for testing

pkg/azure/api/validation/validation_test.go

@@ -96,7 +96,7 @@ func TestValidateProviderSecret(t *testing.T) {
 
 	g := NewWithT(t)
 	for _, entry := range table {
-		t.Run(entry.description, func(t *testing.T) {
+		t.Run(entry.description, func(_ *testing.T) {
 			secret := createSecret(entry.clientID, entry.clientSecret, entry.subscriptionID, entry.tenantID, entry.testUserData)
 			errList := ValidateProviderSecret(secret)
 			g.Expect(len(errList)).To(Equal(entry.expectedErrors))
@@ -141,7 +141,7 @@ func TestValidateSubnetInfo(t *testing.T) {
 	}
 	g := NewWithT(t)
 	for _, entry := range table {
-		t.Run(entry.description, func(t *testing.T) {
+		t.Run(entry.description, func(_ *testing.T) {
 			subnetInfo := api.AzureSubnetInfo{
 				VnetName:   entry.vnetName,
 				SubnetName: entry.subnetName,
@@ -220,7 +220,7 @@ func TestValidateOSDisk(t *testing.T) {
 
 	g := NewWithT(t)
 	for _, entry := range table {
-		t.Run(entry.description, func(t *testing.T) {
+		t.Run(entry.description, func(_ *testing.T) {
 			errList := validateOSDisk(entry.osDisk, fldPath)
 			g.Expect(len(errList)).To(Equal(entry.expectedErrors))
 			if entry.matcher != nil {
@@ -250,7 +250,7 @@ func TestValidateOSProfile(t *testing.T) {
 
 	g := NewWithT(t)
 	for _, entry := range table {
-		t.Run(entry.description, func(t *testing.T) {
+		t.Run(entry.description, func(_ *testing.T) {
 			osProfile := api.AzureOSProfile{
 				ComputerName:  "bingo",
 				AdminUsername: entry.adminUserName,
@@ -311,7 +311,7 @@ func TestValidateDataDisks(t *testing.T) {
 
 	g := NewWithT(t)
 	for _, entry := range table {
-		t.Run(entry.description, func(t *testing.T) {
+		t.Run(entry.description, func(_ *testing.T) {
 			errList := validateDataDisks(entry.disks, fldPath)
 			g.Expect(len(errList)).To(Equal(entry.expectedErrors))
 			if entry.matcher != nil {
@@ -359,7 +359,7 @@ func TestValidateAvailabilityAndScalingConfig(t *testing.T) {
 
 	g := NewWithT(t)
 	for _, entry := range table {
-		t.Run(entry.description, func(t *testing.T) {
+		t.Run(entry.description, func(_ *testing.T) {
 			vmProperties := api.AzureVirtualMachineProperties{
 				AvailabilitySet:        entry.availabilitySet,
 				Zone:                   entry.zone,
@@ -453,7 +453,7 @@ func TestValidateStorageImageRef(t *testing.T) {
 
 	g := NewWithT(t)
 	for _, entry := range table {
-		t.Run(entry.description, func(t *testing.T) {
+		t.Run(entry.description, func(_ *testing.T) {
 			storageImageRef := api.AzureImageReference{
 				ID:                      entry.id,
 				URN:                     entry.urn,

pkg/azure/instrument/instrument_test.go

@@ -18,7 +18,7 @@ import (
 )
 
 var (
-	testErr          = errors.New("test-error")
+	errTest          = errors.New("test-error")
 	defaultErrorCode = strconv.Itoa(int(codes.Internal))
 	testStatusErr    = status.New(codes.InvalidArgument, "test-status-error")
 )
@@ -30,15 +30,15 @@ func TestAPIMetricRecorderFn(t *testing.T) {
 		name string
 		err  error
 	}{
-		{"assert that function captures failed API request count when the error is not nil", testErr},
+		{"assert that function captures failed API request count when the error is not nil", errTest},
 		{"assert that function captures successful API request count when the error is nil", nil},
 	}
 	g := NewWithT(t)
 	reg := prometheus.NewRegistry()
 	g.Expect(reg.Register(metrics.APIRequestCount)).To(Succeed())
 	g.Expect(reg.Register(metrics.APIFailedRequestCount)).To(Succeed())
 	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
+		t.Run(tc.name, func(_ *testing.T) {
 			defer metrics.APIRequestCount.Reset()
 			defer metrics.APIFailedRequestCount.Reset()
 			defer metrics.APIRequestDuration.Reset()
@@ -61,15 +61,15 @@ func TestDriverAPIMetricRecorderFn(t *testing.T) {
 		name string
 		err  error
 	}{
-		{"assert that function captures failed driver API request with default error code for internal error when there is an error", testErr},
+		{"assert that function captures failed driver API request with default error code for internal error when there is an error", errTest},
 		{"assert that function captures failed driver API request with error code from status.Status on error", testStatusErr},
 		{"assert that function captures successful driver API request count when the error is nil", nil},
 	}
 	g := NewWithT(t)
 	reg := prometheus.NewRegistry()
 	g.Expect(reg.Register(metrics.DriverFailedAPIRequests)).To(Succeed())
 	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
+		t.Run(tc.name, func(_ *testing.T) {
 			defer metrics.DriverFailedAPIRequests.Reset()
 			_ = deferredMetricsRecorderInvoker(tc.err != nil, isStatusErr(tc.err), DriverAPIMetricRecorderFn)
 			if tc.err != nil {
@@ -111,7 +111,7 @@ func deferredMetricsRecorderInvoker(shouldReturnErr bool, isStatusErr bool, fn r
 		if isStatusErr {
 			err = testStatusErr
 		} else {
-			err = testErr
+			err = errTest
 		}
 	}
 	return

pkg/azure/provider/helpers/driver.go

@@ -190,7 +190,7 @@ func DeleteVirtualMachine(ctx context.Context, vmAccess *armcompute.VirtualMachi
 
 // IsVirtualMachineInTerminalState checks if the provisioningState of the VM is set to Failed.
 func IsVirtualMachineInTerminalState(vm *armcompute.VirtualMachine) bool {
-	return vm.Properties != nil && vm.Properties.ProvisioningState != nil && strings.ToLower(*vm.Properties.ProvisioningState) == strings.ToLower(utils.ProvisioningStateFailed)
+	return vm.Properties != nil && vm.Properties.ProvisioningState != nil && strings.EqualFold(*vm.Properties.ProvisioningState, utils.ProvisioningStateFailed)
 }
 
 // CanUpdateVirtualMachine checks if the VM is not in terminal state and if there are no data disks marked for detachment.

pkg/azure/provider/helpers/machineclass_test.go

@@ -58,7 +58,7 @@ func TestDecodeMachineSetConfig(t *testing.T) {
 	providerSpec.Properties.Zone = nil
 
 	for _, entry := range table {
-		t.Run(entry.description, func(t *testing.T) {
+		t.Run(entry.description, func(_ *testing.T) {
 			providerSpec.Properties.MachineSet = &entry.machineSetConfig
 			machineClass, err := fakes.CreateMachineClass(providerSpec, nil)
 			g.Expect(err).To(BeNil())

pkg/azure/provider/helpers/resourcegraphprocessor.go

@@ -118,10 +118,8 @@ func findMatchingDataDiskNameSuffix(dataDiskName string, dataDiskNameSuffixes se
 func getDataDiskNameSuffixes(providerSpec api.AzureProviderSpec) sets.Set[string] {
 	dataDiskNameSuffixes := sets.New[string]()
 	dataDisks := providerSpec.Properties.StorageProfile.DataDisks
-	if dataDisks != nil {
-		for _, dataDisk := range dataDisks {
-			dataDiskNameSuffixes.Insert(utils.GetDataDiskNameSuffix(dataDisk))
-		}
+	for _, dataDisk := range dataDisks {
+		dataDiskNameSuffixes.Insert(utils.GetDataDiskNameSuffix(dataDisk))
 	}
 	return dataDiskNameSuffixes
 }