general cleanup inspired by similar cleanup in Kargo proper (#257)

Signed-off-by: Kent Rancourt <[email protected]>

.github/workflows/ci.yaml

@@ -13,7 +13,7 @@ jobs:
   test-unit:
     runs-on: ubuntu-latest
     container:
-      image: golang:1.20.7-bookworm
+      image: golang:1.22.1-bookworm
     steps:
     - name: Checkout code
       uses: actions/checkout@v3
@@ -33,7 +33,7 @@ jobs:
   lint:
     runs-on: ubuntu-latest
     container:
-      image: golang:1.20.7-bookworm
+      image: golang:1.22.1-bookworm
     steps:
     - name: Checkout code
       uses: actions/checkout@v3
@@ -45,8 +45,10 @@ jobs:
           ${{ runner.os }}-go-
     - name: golangci-lint
       uses: golangci/golangci-lint-action@v3
+      env:
+        GOFLAGS: -buildvcs=false
       with:
-        version: v1.53.3
+        version: v1.56.2
 
   build-image:
     needs: [test-unit, lint]

.golangci.yaml

@@ -1,34 +1,42 @@
 run:
-  go: "1.20"
+  go: "1.22"
   deadline: 10m
 
 linters:
   disable-all: true
   enable:
-  - bodyclose
-  - errcheck
-  - exportloopref
-  - forcetypeassert
-  - gci
-  - goconst
-  - gofmt
-  - goimports
-  - gosec
-  - gosimple
-  - govet
-  - ineffassign
-  - lll
-  - misspell
-  - nakedret
-  - revive
-  - staticcheck
-  - typecheck
-  - unconvert
-  - unparam
-  - unused
+    - bodyclose
+    - depguard
+    - errcheck
+    - exportloopref
+    - forcetypeassert
+    - gci
+    - goconst
+    - gofmt
+    - goimports
+    - gosec
+    - gosimple
+    - govet
+    - ineffassign
+    - lll
+    - misspell
+    - nakedret
+    - revive
+    - staticcheck
+    - typecheck
+    - unconvert
+    - unparam
+    - unused
 
 # all available settings for linters we enable
 linters-settings:
+  depguard:
+    rules:
+      main:
+        deny:
+          # avoid reintroduction of `github.com/pkg/errors` package
+          - pkg: "github.com/pkg/errors"
+            desc: Should use `errors` package from the standard library
   errcheck:
     # report about not checking of errors in type assertions: `a := b.(MyStruct)`;
     # default is false: such cases aren't reported by default.
@@ -47,6 +55,7 @@ linters-settings:
     skip-generated: true
     custom-order: true
   goconst:
+    ignore-tests: true
     # minimal length of string constant, 3 by default
     min-len: 3
     # minimal occurrences count to trigger, 3 by default
@@ -94,3 +103,57 @@ linters-settings:
     # if it's called for subdir of a project it can't find funcs usages. All text editor integrations
     # with golangci-lint call it on a directory with the changed file.
     check-exported: false
+  revive:
+    # based on https://github.com/mgechev/revive#recommended-configuration
+    ignore-generated-header: false
+    severity: warning
+    confidence: 0.8
+    error-code: 0
+    warning-code: 0
+    # https://github.com/mgechev/revive/blob/master/RULES_DESCRIPTIONS.md
+    rules:
+      # Deny naked return always
+      - name: bare-return
+        severity: error
+      - name: blank-imports
+      - name: bool-literal-in-expr
+      - name: context-as-argument
+      - name: context-keys-type
+      - name: datarace
+      - name: defer
+      - name: early-return
+      - name: error-naming
+      - name: error-return
+      - name: error-strings
+      - name: errorf
+      - name: exported
+      - name: if-return
+      - name: import-shadowing
+      - name: indent-error-flow
+      - name: increment-decrement
+      - name: range
+      - name: receiver-naming
+      - name: redefines-builtin-id
+      - name: superfluous-else
+      - name: time-equal
+      - name: time-naming
+      - name: var-declaration
+      - name: unexported-naming
+      # Handle error always
+      - name: unhandled-error
+        severity: error
+        # Ignore following functions
+        arguments:
+          - fmt.Print
+          - fmt.Fprintf
+          - fmt.Fprintln
+          - fmt.Printf
+          - fmt.Println
+      - name: unused-parameter
+      - name: use-any
+      - name: useless-break
+      - name: waitgroup-by-value
+      # Explicitly disabled rules
+      - name: var-naming
+        # Disable because it conflicts with protobuf naming convention like "Id"
+        disabled: true

Dockerfile

@@ -1,4 +1,4 @@
-FROM --platform=$BUILDPLATFORM golang:1.20.7-bookworm as builder
+FROM --platform=$BUILDPLATFORM golang:1.22.1-bookworm as builder
 
 ARG TARGETOS
 ARG TARGETARCH
@@ -39,7 +39,7 @@ RUN GOOS=${TARGETOS} GOARCH=${TARGETARCH} go build \
     && cd bin \
     && ln -s kargo-render kargo-render-action
 
-FROM alpine:3.15.4 as final
+FROM alpine:3.19.1 as final
 
 RUN apk update \
     && apk add git openssh-client \

Dockerfile.dev

@@ -1,8 +1,8 @@
-FROM golang:1.20.7-bookworm
+FROM golang:1.22.1-bookworm
 
 ARG TARGETARCH
 
-ARG GOLANGCI_LINT_VERSION=1.53.3
+ARG GOLANGCI_LINT_VERSION=1.56.2
 
 RUN cd /usr/local/bin \
     && curl -sSfL https://github.com/golangci/golangci-lint/releases/download/v${GOLANGCI_LINT_VERSION}/golangci-lint-${GOLANGCI_LINT_VERSION}-linux-${TARGETARCH}.tar.gz \

branches.go

@@ -7,7 +7,6 @@ import (
 	"strings"
 
 	"github.com/ghodss/yaml"
-	"github.com/pkg/errors"
 
 	"github.com/akuity/kargo-render/internal/file"
 )
@@ -34,20 +33,22 @@ func loadBranchMetadata(repoPath string) (*branchMetadata, error) {
 		"metadata.yaml",
 	)
 	if exists, err := file.Exists(path); err != nil {
-		return nil, errors.Wrap(
+		return nil, fmt.Errorf(
+			"error checking for existence of branch metadata: %w",
 			err,
-			"error checking for existence of branch metadata",
 		)
 	} else if !exists {
 		return nil, nil
 	}
 	bytes, err := os.ReadFile(path)
 	if err != nil {
-		return nil, errors.Wrap(err, "error reading branch metadata")
+		return nil, fmt.Errorf("error reading branch metadata: %w", err)
 	}
 	md := &branchMetadata{}
-	err = yaml.Unmarshal(bytes, md)
-	return md, errors.Wrap(err, "error unmarshaling branch metadata")
+	if err = yaml.Unmarshal(bytes, md); err != nil {
+		return nil, fmt.Errorf("error unmarshaling branch metadata: %w", err)
+	}
+	return md, nil
 }
 
 // writeBranchMetadata attempts to marshal the provided BranchMetadata and write
@@ -56,17 +57,20 @@ func writeBranchMetadata(md branchMetadata, repoPath string) error {
 	bkDir := filepath.Join(repoPath, ".kargo-render")
 	// Ensure the existence of the directory
 	if err := os.MkdirAll(bkDir, 0755); err != nil {
-		return errors.Wrapf(err, "error ensuring existence of directory %q", bkDir)
+		return fmt.Errorf("error ensuring existence of directory %q: %w", bkDir, err)
 	}
 	path := filepath.Join(bkDir, "metadata.yaml")
 	bytes, err := yaml.Marshal(md)
 	if err != nil {
-		return errors.Wrap(err, "error marshaling branch metadata")
+		return fmt.Errorf("error marshaling branch metadata: %w", err)
 	}
-	return errors.Wrap(
-		os.WriteFile(path, bytes, 0644), // nolint: gosec
-		"error writing branch metadata",
-	)
+	if err = os.WriteFile(path, bytes, 0644); err != nil { // nolint: gosec
+		return fmt.Errorf(
+			"error writing branch metadata: %w",
+			err,
+		)
+	}
+	return nil
 }
 
 func switchToTargetBranch(rc requestContext) error {
@@ -75,34 +79,34 @@ func switchToTargetBranch(rc requestContext) error {
 	// Check if the target branch exists on the remote
 	targetBranchExists, err := rc.repo.RemoteBranchExists(rc.request.TargetBranch)
 	if err != nil {
-		return errors.Wrap(err, "error checking for existence of target branch")
+		return fmt.Errorf("error checking for existence of target branch: %w", err)
 	}
 
 	if targetBranchExists {
 		logger.Debug("target branch exists on remote")
 		if err = rc.repo.Checkout(rc.request.TargetBranch); err != nil {
-			return errors.Wrap(err, "error checking out target branch")
+			return fmt.Errorf("error checking out target branch: %w", err)
 		}
 		logger.Debug("checked out target branch")
 		return nil
 	}
 
 	logger.Debug("target branch does not exist on remote")
 	if err = rc.repo.CreateOrphanedBranch(rc.request.TargetBranch); err != nil {
-		return errors.Wrap(err, "error creating new target branch")
+		return fmt.Errorf("error creating new target branch: %w", err)
 	}
 	logger.Debug("created target branch")
 	if err =
 		writeBranchMetadata(branchMetadata{}, rc.repo.WorkingDir()); err != nil {
-		return errors.Wrap(err, "error writing blank target branch metadata")
+		return fmt.Errorf("error writing blank target branch metadata: %w", err)
 	}
 	logger.Debug("wrote blank target branch metadata")
 	if err = rc.repo.AddAllAndCommit("Initial commit"); err != nil {
-		return errors.Wrap(err, "error making initial commit to new target branch")
+		return fmt.Errorf("error making initial commit to new target branch: %w", err)
 	}
 	logger.Debug("made initial commit to new target branch")
 	if err = rc.repo.Push(); err != nil {
-		return errors.Wrap(err, "error pushing new target branch to remote")
+		return fmt.Errorf("error pushing new target branch to remote: %w", err)
 	}
 	logger.Debug("pushed new target branch to remote")
 
@@ -129,17 +133,17 @@ func switchToCommitBranch(rc requestContext) (string, error) {
 		commitBranchExists, err := rc.repo.RemoteBranchExists(commitBranch)
 		if err != nil {
 			return "",
-				errors.Wrap(err, "error checking for existence of commit branch")
+				fmt.Errorf("error checking for existence of commit branch: %w", err)
 		}
 		if commitBranchExists {
 			logger.Debug("commit branch exists on remote")
 			if err = rc.repo.Checkout(commitBranch); err != nil {
-				return "", errors.Wrap(err, "error checking out commit branch")
+				return "", fmt.Errorf("error checking out commit branch: %w", err)
 			}
 			logger.Debug("checked out commit branch")
 		} else {
 			if err := rc.repo.CreateChildBranch(commitBranch); err != nil {
-				return "", errors.Wrap(err, "error creating child of target branch")
+				return "", fmt.Errorf("error creating child of target branch: %w", err)
 			}
 			logger.Debug("created commit branch")
 		}
@@ -150,7 +154,7 @@ func switchToCommitBranch(rc requestContext) (string, error) {
 		rc.repo.WorkingDir(),
 		rc.target.branchConfig.PreservedPaths,
 	); err != nil {
-		return "", errors.Wrap(err, "error cleaning commit branch")
+		return "", fmt.Errorf("error cleaning commit branch: %w", err)
 	}
 	logger.Debug("cleaned commit branch")
 

branches_test.go

@@ -15,7 +15,7 @@ func TestLoadBranchMetadata(t *testing.T) {
 	testCases := []struct {
 		name       string
 		setup      func() string
-		assertions func(*branchMetadata, error)
+		assertions func(*testing.T, *branchMetadata, error)
 	}{
 		{
 			name: "metadata does not exist",
@@ -24,7 +24,7 @@ func TestLoadBranchMetadata(t *testing.T) {
 				require.NoError(t, err)
 				return repoDir
 			},
-			assertions: func(md *branchMetadata, err error) {
+			assertions: func(t *testing.T, md *branchMetadata, err error) {
 				require.NoError(t, err)
 				require.Nil(t, md)
 			},
@@ -45,7 +45,7 @@ func TestLoadBranchMetadata(t *testing.T) {
 				require.NoError(t, err)
 				return repoDir
 			},
-			assertions: func(_ *branchMetadata, err error) {
+			assertions: func(t *testing.T, _ *branchMetadata, err error) {
 				require.Error(t, err)
 				require.Contains(t, err.Error(), "error unmarshaling branch metadata")
 			},
@@ -66,15 +66,15 @@ func TestLoadBranchMetadata(t *testing.T) {
 				require.NoError(t, err)
 				return repoDir
 			},
-			assertions: func(_ *branchMetadata, err error) {
+			assertions: func(t *testing.T, _ *branchMetadata, err error) {
 				require.NoError(t, err)
 			},
 		},
 	}
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
 			md, err := loadBranchMetadata(testCase.setup())
-			testCase.assertions(md, err)
+			testCase.assertions(t, md, err)
 		})
 	}
 }
@@ -247,11 +247,11 @@ func createDummyCommitBranchDir(dirCount, fileCount int) (string, error) {
 	}
 	// Add some dummy files
 	for i := 0; i < fileCount; i++ {
-		file, err := os.Create(filepath.Join(dir, fmt.Sprintf("file-%d", i)))
+		f, err := os.Create(filepath.Join(dir, fmt.Sprintf("file-%d", i)))
 		if err != nil {
 			return dir, err
 		}
-		if err = file.Close(); err != nil {
+		if err = f.Close(); err != nil {
 			return dir, err
 		}
 	}