Update integration test workflow and add golangci lint check (#2197)

* Update integration test workflow Signed-off-by: Yi Chen <github@chenyicn.net> * Update golangci lint config Signed-off-by: Yi Chen <github@chenyicn.net> --------- Signed-off-by: Yi Chen <github@chenyicn.net>
kubeflow · Oct 8, 2024 · 143b16e · 143b16e
1 parent 1972fb7
commit 143b16e
Show file tree

Hide file tree

Showing 21 changed files with 200 additions and 121 deletions.
diff --git a/.github/workflows/integration.yaml b/.github/workflows/integration.yaml
@@ -16,39 +16,71 @@ concurrency:
   cancel-in-progress: true
 
 jobs:
-  build-api-docs:
+  code-check:
     runs-on: ubuntu-latest
     steps:
       - name: Checkout source code
         uses: actions/checkout@v4
+        with:
+          fetch-depth: 0
 
       - name: Set up Go
         uses: actions/setup-go@v5
         with:
           go-version-file: go.mod
 
-      - name: The API documentation hasn't changed
+      - name: Run go mod tidy
         run: |
-          make build-api-docs
-          if ! git diff --quiet -- docs/api-docs.md; then
-            echo "Need to re-run 'make build-api-docs' and commit the changes"
-            git diff -- docs/api-docs.md;
+          go mod tidy
+          if ! git diff --quiet; then
+            echo "Please run 'go mod tidy' and commit the changes."
+            git diff
             false
           fi
 
-  build-sparkctl:
+      - name: Run go fmt check
+        run: |
+          make go-fmt
+          if ! git diff --quiet; then
+            echo "Need to re-run 'make go-fmt' and commit the changes."
+            git diff
+            false
+          fi
+
+      - name: Run go vet check
+        run: |
+          make go-vet
+          if ! git diff --quiet; then
+            echo "Need to re-run 'make go-vet' and commit the changes."
+            git diff
+            false
+          fi
+
+      - name: Run golangci-lint
+        run: |
+          make go-lint
+
+  build-api-docs:
     runs-on: ubuntu-latest
     steps:
       - name: Checkout source code
         uses: actions/checkout@v4
+        with:
+          fetch-depth: 0
 
       - name: Set up Go
         uses: actions/setup-go@v5
         with:
           go-version-file: go.mod
 
-      - name: build sparkctl
-        run: make build-sparkctl
+      - name: Build API docs
+        run: |
+          make build-api-docs
+          if ! git diff --quiet; then
+            echo "Need to re-run 'make build-api-docs' and commit the changes."
+            git diff
+            false
+          fi
 
   build-spark-operator:
     runs-on: ubuntu-latest
@@ -63,18 +95,28 @@ jobs:
         with:
           go-version-file: go.mod
 
-      - name: Run go fmt check
-        run: make go-fmt
-
-      - name: Run go vet check
-        run: make go-vet
-
-      - name: Run unit tests
+      - name: Run go unit tests
         run: make unit-test
 
       - name: Build Spark operator
         run: make build-operator
 
+  build-sparkctl:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout source code
+        uses: actions/checkout@v4
+        with:
+          fetch-depth: 0
+
+      - name: Set up Go
+        uses: actions/setup-go@v5
+        with:
+          go-version-file: go.mod
+
+      - name: Build sparkctl
+        run: make build-sparkctl
+
   build-helm-chart:
     runs-on: ubuntu-latest
     steps:
@@ -128,7 +170,7 @@ jobs:
         run: |
           make helm-docs
           if ! git diff --quiet -- charts/spark-operator-chart/README.md; then
-            echo "Need to re-run 'make helm-docs' and commit the changes"
+            echo "Need to re-run 'make helm-docs' and commit the changes."
             false
           fi
 

diff --git a/.golangci.yaml b/.golangci.yaml
@@ -1,39 +1,66 @@
 run:
-  deadline: 5m
+  # Timeout for analysis, e.g. 30s, 5m.
+  # Default: 1m
+  timeout: 1m
 
 linters:
+  # Enable specific linters.
+  # https://golangci-lint.run/usage/linters/#enabled-by-default
   enable:
-  - revive
-  - gci
-  - depguard
-  - godot
-  - testifylint
-  - unconvert
+    # Detects places where loop variables are copied.
+    - copyloopvar
+    # Checks for duplicate words in the source code.
+    - dupword
+    # Tool for detection of FIXME, TODO and other comment keywords.
+    # - godox
+    # Check import statements are formatted according to the 'goimport' command.
+    - goimports
+    # Enforces consistent import aliases.
+    - importas
+    # Find code that shadows one of Go's predeclared identifiers.
+    - predeclared
+    # Check that struct tags are well aligned.
+    - tagalign
+    # Remove unnecessary type conversions.
+    - unconvert
+    # Checks Go code for unused constants, variables, functions and types.
+    - unused
 
 issues:
-  exclude-rules:
-  # Disable errcheck linter for test files.
-  - path: _test.go
-    linters:
-    - errcheck
+  # Which dirs to exclude: issues from them won't be reported.
+  # Can use regexp here: `generated.*`, regexp is applied on full path,
+  # including the path prefix if one is set.
+  # Default dirs are skipped independently of this option's value (see exclude-dirs-use-default).
+  # "/" will be replaced by current OS file path separator to properly work on Windows.
+  # Default: []
+  exclude-dirs:
+    - sparkctl
+  # Maximum issues count per one linter.
+  # Set to 0 to disable.
+  # Default: 50
+  max-issues-per-linter: 50
+  # Maximum count of issues with the same text.
+  # Set to 0 to disable.
+  # Default: 3
+  max-same-issues: 3
 
 linters-settings:
-  gci:
-    sections:
-      - standard
-      - default
-      - prefix(github.com/kubeflow/spark-operator)
-  depguard:
-    Main:
-      files:
-        - $all
-        - "!$test"
-      listMode: Lax
-      deny:
-        reflect: Please don't use reflect package
-    Test:
-      files:
-        - $test
-      listMode: Lax
-      deny:
-        reflect: Please don't use reflect package
+  importas:
+    # List of aliases
+    alias:
+    - pkg: k8s.io/api/admissionregistration/v1
+      alias: admissionregistrationv1
+    - pkg: k8s.io/api/apps/v1
+      alias: appsv1
+    - pkg: k8s.io/api/batch/v1
+      alias: batchv1
+    - pkg: k8s.io/api/core/v1
+      alias: corev1
+    - pkg: k8s.io/api/extensions/v1beta1
+      alias: extensionsv1beta1
+    - pkg: k8s.io/api/networking/v1
+      alias: networkingv1
+    - pkg: k8s.io/apimachinery/pkg/apis/meta/v1
+      alias: metav1
+    - pkg: sigs.k8s.io/controller-runtime
+      alias: ctrl
diff --git a/Makefile b/Makefile
@@ -55,7 +55,7 @@ KIND_VERSION ?= v0.23.0
 ENVTEST_VERSION ?= release-0.18
 # ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary.
 ENVTEST_K8S_VERSION ?= 1.29.3
-GOLANGCI_LINT_VERSION ?= v1.57.2
+GOLANGCI_LINT_VERSION ?= v1.61.0
 GEN_CRD_API_REFERENCE_DOCS_VERSION ?= v0.3.0
 HELM_VERSION ?= v3.15.3
 HELM_UNITTEST_VERSION ?= 0.5.1
@@ -139,13 +139,13 @@ go-vet: ## Run go vet against code.
 	@echo "Running go vet..."
 	go vet ./...
 
-.PHONY: lint
-lint: golangci-lint ## Run golangci-lint linter.
+.PHONY: go-lint
+go-lint: golangci-lint ## Run golangci-lint linter.
 	@echo "Running golangci-lint run..."
 	$(GOLANGCI_LINT) run
 
-.PHONY: lint-fix
-lint-fix: golangci-lint ## Run golangci-lint linter and perform fixes.
+.PHONY: go-lint-fix
+go-lint-fix: golangci-lint ## Run golangci-lint linter and perform fixes.
 	@echo "Running golangci-lint run --fix..."
 	$(GOLANGCI_LINT) run --fix
 

diff --git a/api/v1beta1/sparkapplication_types.go b/api/v1beta1/sparkapplication_types.go
@@ -70,8 +70,8 @@ type SparkApplicationSpec struct {
 	// spark-submit.
 	// Optional.
 	SparkConf map[string]string `json:"sparkConf,omitempty"`
-	// HadoopConf carries user-specified Hadoop configuration properties as they would use the  the "--conf" option
-	// in spark-submit.  The SparkApplication controller automatically adds prefix "spark.hadoop." to Hadoop
+	// HadoopConf carries user-specified Hadoop configuration properties as they would use the "--conf" option
+	// in spark-submit. The SparkApplication controller automatically adds prefix "spark.hadoop." to Hadoop
 	// configuration properties.
 	// Optional.
 	HadoopConf map[string]string `json:"hadoopConf,omitempty"`

diff --git a/api/v1beta2/defaults.go b/api/v1beta2/defaults.go
@@ -54,7 +54,6 @@ func SetSparkApplicationDefaults(app *SparkApplication) {
 }
 
 func setDriverSpecDefaults(spec *DriverSpec, sparkConf map[string]string) {
-
 	if _, exists := sparkConf["spark.driver.cores"]; !exists && spec.Cores == nil {
 		spec.Cores = new(int32)
 		*spec.Cores = 1

diff --git a/api/v1beta2/defaults_test.go b/api/v1beta2/defaults_test.go
@@ -121,7 +121,6 @@ func TestSetSparkApplicationDefaultsOnFailureRestartPolicyShouldSetDefaultValueF
 }
 
 func TestSetSparkApplicationDefaultsDriverSpecDefaults(t *testing.T) {
-
 	//Case1: Driver config not set.
 	app := &SparkApplication{
 		Spec: SparkApplicationSpec{},

diff --git a/api/v1beta2/doc.go b/api/v1beta2/doc.go
@@ -15,7 +15,6 @@ limitations under the License.
 */
 
 // +k8s:deepcopy-gen=package,register
-// go:generate controller-gen crd:trivialVersions=true paths=. output:dir=.
 
 // Package v1beta2 is the v1beta2 version of the API.
 // +groupName=sparkoperator.k8s.io

diff --git a/api/v1beta2/sparkapplication_types.go b/api/v1beta2/sparkapplication_types.go
@@ -71,8 +71,8 @@ type SparkApplicationSpec struct {
 	// spark-submit.
 	// +optional
 	SparkConf map[string]string `json:"sparkConf,omitempty"`
-	// HadoopConf carries user-specified Hadoop configuration properties as they would use the  the "--conf" option
-	// in spark-submit.  The SparkApplication controller automatically adds prefix "spark.hadoop." to Hadoop
+	// HadoopConf carries user-specified Hadoop configuration properties as they would use the "--conf" option
+	// in spark-submit. The SparkApplication controller automatically adds prefix "spark.hadoop." to Hadoop
 	// configuration properties.
 	// +optional
 	HadoopConf map[string]string `json:"hadoopConf,omitempty"`

diff --git a/cmd/operator/controller/start.go b/cmd/operator/controller/start.go
@@ -248,7 +248,7 @@ func start() {
 
 		// Register kube-schedulers.
 		for _, name := range kubeSchedulerNames {
-			registry.Register(name, kubescheduler.Factory)
+			_ = registry.Register(name, kubescheduler.Factory)
 		}
 
 		schedulerNames := registry.GetRegisteredSchedulerNames()

diff --git a/docs/api-docs.md b/docs/api-docs.md
@@ -1939,8 +1939,8 @@ map[string]string
 </td>
 <td>
 <em>(Optional)</em>
-<p>HadoopConf carries user-specified Hadoop configuration properties as they would use the  the &ldquo;&ndash;conf&rdquo; option
-in spark-submit.  The SparkApplication controller automatically adds prefix &ldquo;spark.hadoop.&rdquo; to Hadoop
+<p>HadoopConf carries user-specified Hadoop configuration properties as they would use the &ldquo;&ndash;conf&rdquo; option
+in spark-submit. The SparkApplication controller automatically adds prefix &ldquo;spark.hadoop.&rdquo; to Hadoop
 configuration properties.</p>
 </td>
 </tr>
@@ -2381,8 +2381,8 @@ map[string]string
 </td>
 <td>
 <em>(Optional)</em>
-<p>HadoopConf carries user-specified Hadoop configuration properties as they would use the  the &ldquo;&ndash;conf&rdquo; option
-in spark-submit.  The SparkApplication controller automatically adds prefix &ldquo;spark.hadoop.&rdquo; to Hadoop
+<p>HadoopConf carries user-specified Hadoop configuration properties as they would use the &ldquo;&ndash;conf&rdquo; option
+in spark-submit. The SparkApplication controller automatically adds prefix &ldquo;spark.hadoop.&rdquo; to Hadoop
 configuration properties.</p>
 </td>
 </tr>

diff --git a/internal/controller/scheduledsparkapplication/controller_test.go b/internal/controller/scheduledsparkapplication/controller_test.go
@@ -21,6 +21,7 @@ import (
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"

diff --git a/internal/scheduler/volcano/scheduler_test.go b/internal/scheduler/volcano/scheduler_test.go
@@ -92,7 +92,6 @@ func TestGetDriverResource(t *testing.T) {
 }
 
 func TestGetExecutorResource(t *testing.T) {
-
 	oneCore := int32(1)
 	oneCoreStr := "1"
 	oneGB := "1024m"

diff --git a/internal/scheduler/yunikorn/scheduler.go b/internal/scheduler/yunikorn/scheduler.go
@@ -21,7 +21,7 @@ import (
 	"fmt"
 	"maps"
 
-	v1 "k8s.io/api/core/v1"
+	corev1 "k8s.io/api/core/v1"
 
 	"github.com/kubeflow/spark-operator/api/v1beta2"
 	"github.com/kubeflow/spark-operator/internal/scheduler"
@@ -47,13 +47,13 @@ const (
 // This struct has been defined separately rather than imported so that tags can be included for JSON marshalling
 // https://github.com/apache/yunikorn-k8shim/blob/207e4031c6484c965fca4018b6b8176afc5956b4/pkg/cache/amprotocol.go#L47-L56
 type taskGroup struct {
-	Name         string            `json:"name"`
-	MinMember    int32             `json:"minMember"`
-	MinResource  map[string]string `json:"minResource,omitempty"`
-	NodeSelector map[string]string `json:"nodeSelector,omitempty"`
-	Tolerations  []v1.Toleration   `json:"tolerations,omitempty"`
-	Affinity     *v1.Affinity      `json:"affinity,omitempty"`
-	Labels       map[string]string `json:"labels,omitempty"`
+	Name         string              `json:"name"`
+	MinMember    int32               `json:"minMember"`
+	MinResource  map[string]string   `json:"minResource,omitempty"`
+	NodeSelector map[string]string   `json:"nodeSelector,omitempty"`
+	Tolerations  []corev1.Toleration `json:"tolerations,omitempty"`
+	Affinity     *corev1.Affinity    `json:"affinity,omitempty"`
+	Labels       map[string]string   `json:"labels,omitempty"`
 }
 
 type Scheduler struct{}