Fix failing golangci-lint-check

Set up a timeout for golangci-lint checks. Address the majority of lint issues by simplifying code, sorting import orders, removing unused variables, applying missing Go documentation, fixing variable names, correcting typos, and improving the usage of 'switch' statements, among other things. Additionally, introduce a list of lint issues for the community to handle.

Signed-off-by: Oleksandr Andriienko <[email protected]>

.gitignore

@@ -1,6 +1,7 @@
 # IDE
 .vscode
 .idea
+.bin
 
 # Testing
 temp

.golangci.yml

@@ -26,6 +26,49 @@ issues:
       linters:
         - errcheck
         - gosec
+    # TODO: should be fixed by community
+    - path: pkg/api/server/config/config.go
+      linters:
+        - revive
+    - path: pkg/api/server/v1alpha2/result/result.go
+      linters:
+        - staticcheck
+    - path: pkg/api/server/v1alpha2/record/record.go
+      linters:
+        - staticcheck
+    - path: pkg/api/server/v1alpha2/records_test.go
+      linters:
+        - staticcheck
+    - path: cmd/api/main.go
+      linters:
+        - gocritic
+        - gosec
+    - path: cmd/watcher/main.go
+      linters:
+        - gocritic
+        - staticcheck
+    - path: pkg/watcher/logs/client.go
+      linters:
+        - staticcheck
+    - path: pkg/internal/test/clients.go
+      linters:
+        - gosec
+    - path: pkg/api/server/cel2sql/type_coercion.go
+      linters:
+        - gocritic
+    - path: pkg/api/server/v1alpha2/results_test.go
+      linters:
+        - staticcheck
+    - path: pkg/api/server/v1alpha2/results.go
+      linters:
+        - staticcheck
+    - path: pkg/api/server/v1alpha2/records.go
+      linters:
+        - staticcheck
+    - path: pkg/api/server/cel2sql/interpreter.go
+      linters:
+        - staticcheck
+        - revive
   max-issues-per-linter: 0
   max-same-issues: 0
   include:

Makefile

@@ -85,7 +85,7 @@ golangci-lint: | $(GOLANGCILINT) ; $(info $(M) running golangci-lint…) @ ## Ru
 
 .PHONY: golangci-lint-check
 golangci-lint-check: | $(GOLANGCILINT) ; $(info $(M) Testing if golint has been done…) @ ## Run golangci-lint for build tests CI job
-	$Q $(GOLANGCILINT) run -j 1 --color=never
+	$Q $(GOLANGCILINT) run -j 1 --color=never --deadline 15m
 
 GOIMPORTS = $(BIN)/goimports
 $(BIN)/goimports: PACKAGE=golang.org/x/tools/cmd/goimports

cmd/api/main.go

@@ -19,15 +19,16 @@ package main
 import (
 	"context"
 	"fmt"
-	"github.com/tektoncd/results/pkg/api/server/v1alpha2/auth/impersonation"
-	"golang.org/x/net/http2"
-	"golang.org/x/net/http2/h2c"
-	"google.golang.org/grpc/credentials/insecure"
 	"net/http"
 	"path"
 	"strings"
 	"time"
 
+	"github.com/tektoncd/results/pkg/api/server/v1alpha2/auth/impersonation"
+	"golang.org/x/net/http2"
+	"golang.org/x/net/http2/h2c"
+	"google.golang.org/grpc/credentials/insecure"
+
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
 
@@ -64,7 +65,9 @@ func main() {
 	serverConfig := config.Get()
 
 	log := logger.Get(serverConfig.LOG_LEVEL)
-	defer log.Sync()
+	// This defer statement will be executed at the end of the application lifecycle, so we do not lose
+	// any data in the event of an unhandled error.
+	defer log.Sync() //nolint:errcheck
 
 	// Load server TLS
 	certFile := path.Join(serverConfig.TLS_PATH, "tls.crt")
@@ -91,7 +94,7 @@ func main() {
 		gormConfig.Logger = gormlogger.Default.LogMode(gormlogger.Silent)
 	}
 	// Retry database connection, sometimes the database is not ready to accept connection
-	wait.PollImmediate(10*time.Second, 2*time.Minute, func() (bool, error) {
+	err = wait.PollImmediate(10*time.Second, 2*time.Minute, func() (bool, error) {
 		db, err = gorm.Open(postgres.Open(dbURI), gormConfig)
 		if err != nil {
 			log.Warnf("Error connecting to database (retrying in 10s): %v", err)
@@ -229,9 +232,8 @@ func main() {
 	log.Infof("gRPC and REST server listening on: %s", serverConfig.SERVER_PORT)
 	if tlsError != nil {
 		log.Fatal(http.ListenAndServe(":"+serverConfig.SERVER_PORT, grpcHandlerFunc(gs, httpMux)))
-	} else {
-		log.Fatal(http.ListenAndServeTLS(":"+serverConfig.SERVER_PORT, certFile, keyFile, grpcHandlerFunc(gs, httpMux)))
 	}
+	log.Fatal(http.ListenAndServeTLS(":"+serverConfig.SERVER_PORT, certFile, keyFile, grpcHandlerFunc(gs, httpMux)))
 }
 
 // grpcHandlerFunc forwards the request to gRPC server based on the Content-Type header.

cmd/api/main_test.go

@@ -3,14 +3,15 @@ package main
 import (
 	"bytes"
 	"context"
+	"io"
+	"strings"
+	"testing"
+
 	"github.com/golang-jwt/jwt/v4"
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap"
 	"go.uber.org/zap"
 	"go.uber.org/zap/zapcore"
 	"google.golang.org/grpc/metadata"
-	"io"
-	"strings"
-	"testing"
 )
 
 func Test_determineAuth(t *testing.T) {

cmd/watcher/main.go

@@ -50,7 +50,8 @@ import (
 
 const (
 	// Service Account token path. See https://kubernetes.io/docs/tasks/access-application-cluster/access-cluster/#accessing-the-api-from-a-pod
-	podTokenPath = "/var/run/secrets/kubernetes.io/serviceaccount/token"
+	// This is a fixed path which does not contain a hard-coded secret or credential
+	podTokenPath = "/var/run/secrets/kubernetes.io/serviceaccount/token" //nolint:gosec
 )
 
 var (

pkg/api/server/cel2sql/functions.go

@@ -85,11 +85,7 @@ func (i *interpreter) translateToBinaryCall(expr *exprpb.Expr_Call, infixTerm st
 		return err
 	}
 	fmt.Fprintf(&i.query, " %s ", infixTerm)
-	if err := i.interpretExpr(expr.Args[0]); err != nil {
-		return err
-	}
-
-	return nil
+	return i.interpretExpr(expr.Args[0])
 }
 
 func (i *interpreter) translateToExtractFunctionCall(expr *exprpb.Expr_Call, field string, decrementReturnValue bool) error {

pkg/api/server/cel2sql/interpreter.go

@@ -77,7 +77,7 @@ func (i *interpreter) interpretExpr(expr *exprpb.Expr) error {
 		return i.interpretCallExpr(id, expr)
 
 	case *exprpb.Expr_ListExpr:
-		return i.interpretListExpr(id, node)
+		return i.interpretListExpr(node)
 
 	default:
 		return i.unsupportedExprError(id, "")
@@ -266,7 +266,7 @@ func (i *interpreter) interpretSelectExpr(id int64, expr *exprpb.Expr_SelectExpr
 		return nil
 	}
 
-	return fmt.Errorf("%w. %s: not recognized field.", i.unsupportedExprError(id, "select"), reversedFields[0])
+	return fmt.Errorf("%w. %s: not recognized field", i.unsupportedExprError(id, "select"), reversedFields[0])
 }
 
 func (i *interpreter) interpretCallExpr(id int64, expr *exprpb.Expr) error {
@@ -349,7 +349,7 @@ func (i *interpreter) interpretBinaryCallExpr(expr *exprpb.Expr) error {
 	return nil
 }
 
-func (i *interpreter) interpretListExpr(id int64, expr *exprpb.Expr_ListExpr) error {
+func (i *interpreter) interpretListExpr(expr *exprpb.Expr_ListExpr) error {
 	elements := expr.ListExpr.GetElements()
 	i.query.WriteString("(")
 	for index, elem := range elements {

pkg/api/server/config/config.go

@@ -1,8 +1,9 @@
 package config
 
 import (
-	"github.com/spf13/viper"
 	"log"
+
+	"github.com/spf13/viper"
 )
 
 type Config struct {

pkg/api/server/db/errors/errors.go

@@ -61,8 +61,7 @@ func Wrap(err error) error {
 // See https://pkg.go.dev/gorm.io/[email protected]#pkg-variables for list of
 // errors.
 func gormCode(err error) (codes.Code, bool) {
-	switch {
-	case errors.Is(err, gorm.ErrRecordNotFound):
+	if errors.Is(err, gorm.ErrRecordNotFound) {
 		return codes.NotFound, true
 	}
 	return codes.Unknown, false

pkg/api/server/db/model.go

@@ -91,10 +91,7 @@ func (ann *Annotations) Scan(value interface{}) error {
 	if !ok {
 		return fmt.Errorf("wanted []byte, got %T: %+v", value, value)
 	}
-	if err := json.Unmarshal(bytes, ann); err != nil {
-		return err
-	}
-	return nil
+	return json.Unmarshal(bytes, ann)
 }
 
 // Value returns the value of Annotations for database driver. This implements driver.Valuer.

pkg/api/server/db/pagination/pagination.go

@@ -90,7 +90,7 @@ func (b *Batcher) Update(matched, total int) {
 
 // Next returns the recommended next batch size to query.
 func (b *Batcher) Next() int {
-	n := int(math.Ceil(float64(b.want) / float64(b.ratio)))
+	n := int(math.Ceil(float64(b.want) / b.ratio))
 	if n > b.max {
 		return b.max
 	}

pkg/api/server/logger/logger.go

@@ -1,10 +1,12 @@
 package logger
 
 import (
-	"go.uber.org/zap"
 	"log"
+
+	"go.uber.org/zap"
 )
 
+// Get returns instance sugared zap logger with production configuration
 func Get(level string) *zap.SugaredLogger {
 	config := zap.NewProductionConfig()
 	if len(level) > 0 {

pkg/api/server/v1alpha2/auth/auth.go

@@ -17,14 +17,22 @@ package auth
 import "context"
 
 const (
+	// ResourceResults - api results resource name
 	ResourceResults = "results"
+	// ResourceRecords - api record resource name
 	ResourceRecords = "records"
-	ResourceLogs    = "logs"
+	// ResourceLogs - api logs resource name
+	ResourceLogs = "logs"
 
+	// PermissionCreate - permission name to "create" resource
 	PermissionCreate = "create"
-	PermissionGet    = "get"
-	PermissionList   = "list"
+	// PermissionGet - permission name to "get" resource
+	PermissionGet = "get"
+	// PermissionList - permission name to "list" resource
+	PermissionList = "list"
+	// PermissionDelete - permission name to "delete" resource
 	PermissionDelete = "delete"
+	// PermissionUpdate - permission name to "update" resource
 	PermissionUpdate = "update"
 )
 

pkg/api/server/v1alpha2/auth/impersonation/impersonation.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+
 	"github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
 	"google.golang.org/grpc/metadata"
 	authenticationv1 "k8s.io/api/authentication/v1"
@@ -12,19 +13,24 @@ import (
 	"k8s.io/apiserver/pkg/authentication/user"
 	authorizationclient "k8s.io/client-go/kubernetes/typed/authorization/v1"
 
-	"k8s.io/apiserver/pkg/authentication/serviceaccount"
 	"net/url"
 	"strings"
+
+	"k8s.io/apiserver/pkg/authentication/serviceaccount"
 )
 
+// Impersonation represents component to add Kubernetes impersonation header processing,
+// make impersonation access check and RBAC for Tekton results resources with impersonated user data
 type Impersonation struct {
 	resourceAttributes []authorizationv1.ResourceAttributes
 	userInfo           *user.DefaultInfo
 }
 
 var (
-	ErrorNoImpersonationData     = errors.New("no impersonation data found")
-	ErrorImpersonateUserRequired = errors.New("impersonate user is required to impersonate groups, UID, extra")
+	// ErrNoImpersonationData is an error message for no impersonation data case
+	ErrNoImpersonationData = errors.New("no impersonation data found")
+	// ErrImpersonateUserRequired is an error message about required impersonate user to impersonate another info.
+	ErrImpersonateUserRequired = errors.New("impersonate user is required to impersonate groups, UID, extra")
 )
 
 // NewImpersonation returns an impersonation request if any impersonation data is found, returns error otherwise.
@@ -148,10 +154,9 @@ func (i *Impersonation) parseMetadata(md metadata.MD) error {
 		// Impersonate-User header is mandatory.
 		// https://kubernetes.io/docs/reference/access-authn-authz/authentication/#user-impersonation
 		if hasGroups || hasExtra || hasUID {
-			return ErrorImpersonateUserRequired
-		} else {
-			return ErrorNoImpersonationData
+			return ErrImpersonateUserRequired
 		}
+		return ErrNoImpersonationData
 	}
 
 	return nil
@@ -168,7 +173,7 @@ func unescapeExtraKey(encodedKey string) string {
 // Check checks if the requester has permission to impersonate every resource.
 func (i *Impersonation) Check(ctx context.Context, authorizer authorizationclient.AuthorizationV1Interface, requester string) error {
 	if i.resourceAttributes == nil {
-		return ErrorNoImpersonationData
+		return ErrNoImpersonationData
 	}
 	for _, resourceAttribute := range i.resourceAttributes {
 		sar, err := authorizer.SubjectAccessReviews().Create(ctx, &authorizationv1.SubjectAccessReview{

pkg/api/server/v1alpha2/auth/impersonation/impersonation_test.go

@@ -2,14 +2,15 @@ package impersonation
 
 import (
 	"context"
+	"testing"
+
 	"github.com/google/go-cmp/cmp"
 	"google.golang.org/grpc/metadata"
 	authorizationv1 "k8s.io/api/authorization/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/client-go/kubernetes/fake"
 	test "k8s.io/client-go/testing"
-	"testing"
 )
 
 func TestHeaderMatcher(t *testing.T) {
@@ -56,7 +57,7 @@ func TestHeaderMatcher(t *testing.T) {
 func TestNewImpersonation(t *testing.T) {
 
 	t.Run("missing all impersonation header", func(t *testing.T) {
-		want := ErrorNoImpersonationData
+		want := ErrNoImpersonationData
 		md := metadata.MD{}
 		_, err := NewImpersonation(md)
 		if err != want {
@@ -65,7 +66,7 @@ func TestNewImpersonation(t *testing.T) {
 	})
 
 	t.Run("missing impersonate user header only", func(t *testing.T) {
-		want := ErrorImpersonateUserRequired
+		want := ErrImpersonateUserRequired
 		md := metadata.MD{}
 		md.Append("Impersonate-Group", "authorized-group")
 		_, err := NewImpersonation(md)

pkg/api/server/v1alpha2/auth/nop.go

@@ -20,6 +20,7 @@ import "context"
 // params. Useful for testing or cases where you want to disable auth checks.
 type AllowAll struct{}
 
+// Check does nothing.
 func (AllowAll) Check(context.Context, string, string, string) error {
 	return nil
 }