chore: enable more Go linters

agent/internal/container/container.go

@@ -336,7 +336,7 @@ func (c *Container) finalize(ctx context.Context, err error) {
 	switch err := err.(type) {
 	case nil:
 		stop = aproto.ContainerStopped{Failure: nil}
-	case *aproto.ContainerFailure:
+	case *aproto.ContainerFailureError:
 		stop = aproto.ContainerStopped{Failure: err}
 	default:
 		stop = aproto.ContainerError(aproto.TaskError, err)
@@ -345,7 +345,6 @@ func (c *Container) finalize(ctx context.Context, err error) {
 	if err := c.terminated(ctx, stop); err != nil {
 		c.log.WithError(err).Error("finalizing container")
 	}
-	return
 }
 
 func (c *Container) summary() cproto.Container {

agent/internal/container/container_api_test.go

@@ -50,7 +50,7 @@ func TestContainer(t *testing.T) {
 		signalAtState cproto.State
 		signal        syscall.Signal
 
-		failure *aproto.ContainerFailure
+		failure *aproto.ContainerFailureError
 	}{
 		{
 			name:       "successful command",
@@ -62,7 +62,7 @@ func TestContainer(t *testing.T) {
 			name:       "non-existent image",
 			image:      "lieblos/notanimageipushed",
 			entrypoint: []string{"echo", "hello"},
-			failure: &aproto.ContainerFailure{
+			failure: &aproto.ContainerFailureError{
 				FailureType: aproto.TaskError,
 				ErrMsg:      "repository does not exist or may require 'docker login'",
 			},
@@ -71,7 +71,7 @@ func TestContainer(t *testing.T) {
 			name:       "non-existent command",
 			image:      "python:3.8.16",
 			entrypoint: []string{"badcommandthatdoesntexit"},
-			failure: &aproto.ContainerFailure{
+			failure: &aproto.ContainerFailureError{
 				FailureType: aproto.TaskError,
 				ErrMsg:      "executable file not found in $PATH",
 			},
@@ -80,7 +80,7 @@ func TestContainer(t *testing.T) {
 			name:       "failed command",
 			image:      "python:3.8.16",
 			entrypoint: []string{"ls", "badfile"},
-			failure: &aproto.ContainerFailure{
+			failure: &aproto.ContainerFailureError{
 				FailureType: aproto.ContainerFailed,
 				ErrMsg:      "container failed with non-zero exit code",
 				ExitCode:    (*aproto.ExitCode)(ptrs.Ptr(2)),
@@ -91,7 +91,7 @@ func TestContainer(t *testing.T) {
 			image:         "pytorch/pytorch",
 			entrypoint:    []string{"echo", "hello"},
 			detachAtState: cproto.Pulling,
-			failure: &aproto.ContainerFailure{
+			failure: &aproto.ContainerFailureError{
 				FailureType: aproto.ContainerMissing,
 				ErrMsg:      "container is gone on reattachment",
 			},
@@ -108,7 +108,7 @@ func TestContainer(t *testing.T) {
 			entrypoint:    []string{"echo", "hello"},
 			signalAtState: cproto.Pulling,
 			signal:        syscall.SIGKILL,
-			failure: &aproto.ContainerFailure{
+			failure: &aproto.ContainerFailureError{
 				FailureType: aproto.ContainerAborted,
 				ErrMsg:      "killed before run",
 			},
@@ -119,7 +119,7 @@ func TestContainer(t *testing.T) {
 			entrypoint:    []string{"sleep", "60"},
 			signalAtState: cproto.Running,
 			signal:        syscall.SIGKILL,
-			failure: &aproto.ContainerFailure{
+			failure: &aproto.ContainerFailureError{
 				FailureType: aproto.ContainerFailed,
 				ErrMsg:      "137",
 				ExitCode:    (*aproto.ExitCode)(ptrs.Ptr(137)),
@@ -229,7 +229,7 @@ func TestContainerStatus(t *testing.T) {
 	dockerEventAction := "create"
 	timeoutDuration := 10 * time.Second
 	signalToSend := syscall.SIGKILL
-	expectedFailure := &aproto.ContainerFailure{
+	expectedFailure := &aproto.ContainerFailureError{
 		FailureType: aproto.ContainerAborted,
 		ErrMsg:      "killed before run",
 	}

agent/internal/containers/manager.go

@@ -97,7 +97,7 @@ func (m *Manager) ReattachContainers(
 			m.log.Tracef("container is gone on reattachment %s", cID)
 			ack = aproto.ContainerReattachAck{
 				Container: cproto.Container{ID: cID},
-				Failure: &aproto.ContainerFailure{
+				Failure: &aproto.ContainerFailureError{
 					FailureType: aproto.RestoreError,
 					ErrMsg:      "container is gone on reattachment",
 				},
@@ -110,7 +110,7 @@ func (m *Manager) ReattachContainers(
 				m.log.WithError(err).Tracef("failed to reattach container %s", cID)
 				ack = aproto.ContainerReattachAck{
 					Container: cproto.Container{ID: cID},
-					Failure: &aproto.ContainerFailure{
+					Failure: &aproto.ContainerFailureError{
 						FailureType: aproto.RestoreError,
 						ErrMsg:      err.Error(),
 					},
@@ -182,7 +182,7 @@ func (m *Manager) RevalidateContainers(
 		// Else fallback to a missing message.
 		result = append(result, aproto.ContainerReattachAck{
 			Container: cproto.Container{ID: cid},
-			Failure: &aproto.ContainerFailure{
+			Failure: &aproto.ContainerFailureError{
 				FailureType: aproto.RestoreError,
 				ErrMsg:      "failed to restore container on master blip",
 			},
@@ -325,7 +325,7 @@ func (m *Manager) reattachContainer(
 
 func (m *Manager) recentExit(
 	cID cproto.ID,
-	fallback *aproto.ContainerFailure,
+	fallback *aproto.ContainerFailureError,
 ) *aproto.ContainerStateChanged {
 	var stop *aproto.ContainerStateChanged
 	m.recentExits.Do(func(v any) {

master/.golangci.yml

@@ -84,79 +84,66 @@ linters-settings:
 linters:
   enable-all: true
   disable:
-    - deadcode
-    - exhaustive
-    - exhaustivestruct
-    - funlen
-    - gochecknoglobals
-    - gochecknoinits
-    - gocognit
-    - gocyclo
-    - godox
-    - goerr113
-    - gofumpt
-    - golint
-    - gomnd
-    - maligned
-    - nestif
-    - noctx
-    - scopelint
-    - testpackage
-    - unparam
-    - wsl
-    - varcheck
-
+    # Linters that need triaging. Put all linters here if you upgrade.
     # Below here are new linters from upgrading to 1.43.0. Since we enable all and disable
     # selectively, when we upgrade we get a ton of new linters. For convenience next upgrade,
     # golangci-lint can tell you which linters are enabled:
     #   golangci-lint linters | sed -n '/Enabled/,/Disabled/p'
     # To maintain the same set us linters, disable those in the new set that are not in the old:
     #   comm -13 <(cut -d : -f 1 <oldlinters.txt) <(cut -d : -f 1 <newlinters.txt)
-    - bidichk
-    - contextcheck
-    - cyclop
-    - durationcheck
-    - errname
-    - errorlint
-    - forcetypeassert
-    - gci
-    - gomoddirectives
-    - ifshort
-    - importas
-    - ireturn
-    - makezero
-    - nilerr
-    - nilnil
-    - nlreturn
-    - paralleltest
+
+    # Linters that we should probably enable. Please give each a ticket.
+    - cyclop          # TODO(DET-9951)
+    - errorlint       # TODO(DET-9952)
+    - forcetypeassert # TODO(DET-9953)
+    - wrapcheck       # TODO(DET-9954)
+    - maintidx        # TODO(DET-9956)
+    - gocyclo         # TODO(DET-9957)
+    - gocognit        # TODO(DET-9958)
+    - funlen          # TODO(DET-9959)
+    - nestif          # TODO(DET-9960)
+
+    # Toss up linters.
     - predeclared
     - promlinter
-    - tagliatelle
-    - tenv
     - thelper
-    - tparallel
-    - varnamelen
-    - wastedassign
-    - wrapcheck
-
-    # Below here are new linters from upgrading to 1.45.0. Since we enable all and disable
-    # selectively, when we upgrade we get a ton of new linters.
-    - maintidx
-    - containedctx
-    - decorder
-    - errchkjson
-    - grouper
-
-    # Below here are new linters from upgrading to 1.51.1.
+    - tagliatelle
+    - nilnil
+    - ireturn
+    - contextcheck
     - nonamedreturns
-    - nosnakecase
-    - musttag
     - interfacebloat
-    - gosimple 
-    - prealloc
-    - interfacer
-    - structcheck
-    - nolintlint
-    - usestdlibvars
-    - exhaustruct
+    - wsl
+    - godox
+    - gochecknoinits
+    - goerr113
+    - gomnd
+
+    # Linters that we should probably keep disabled.
+    - errchkjson       # Requiring us to ignore errors (even if they won't be non nil) feels strange.
+    - musttag          # Really buggy now.
+    - prealloc         # Nick thinks this is premature optimization.
+    - varnamelen       # This is overally opinionated.
+    - paralleltest     # I don't understand this linter.
+    - gomoddirectives  # Seems unneeded and just going to make us make exceptions when we need to.
+    - gci              # We aren't using the gci tool.
+    - nolintlint       # Ideally should enable, but gofumpt adds leading space to // nolint for funcs.
+    - nlreturn         # This is overally opinionated.
+    - testpackage      # We don't use seperate test packages.
+    - unparam          # We have a lot of unused parameters.
+    - gochecknoglobals # We have globals currently and don't have an issue with too many.
+    - exhaustive       # We often use switch statements as if statements.
+
+    # Linters that are deprecated / replaced / removed.
+    - nosnakecase      # Replaced by revive(var-naming).
+    - ifshort          # The repository of the linter has been deprecated by the owner.
+    - interfacer       # Linter archived since it can give bad suggestions.
+    - wastedassign     # We already have ineffassign.
+    - scopelint        # Replaced by exportloopref.
+    - exhaustivestruct # Replaced by exhaustruct.
+    - structcheck      # Replaced by unusued.
+    - varcheck         # Replaced by unusued.
+    - deadcode         # Replaced by unusued.
+    - maligned         # Replaced by govet 'fieldalignment'.
+    - golint           # Replaced by revive.
 

master/internal/api_checkpoint.go

@@ -34,9 +34,7 @@ import (
 
 func errCheckpointsNotFound(ids []string) error {
 	tmp := make([]string, len(ids))
-	for i, id := range ids {
-		tmp[i] = id
-	}
+	copy(tmp, ids)
 	sort.Strings(tmp)
 	return api.NotFoundErrs("checkpoints", strings.Join(tmp, ", "), true)
 }

master/internal/api_experiment.go

@@ -1548,8 +1548,8 @@ func (a *apiServer) ContinueExperiment(
 				}
 			}
 			if !hasIncompleteTrials {
-				return status.Error(codes.FailedPrecondition, fmt.Sprint(
-					"experiment has been completed, cannot continue this experiment"))
+				return status.Error(codes.FailedPrecondition,
+					"experiment has been completed, cannot continue this experiment")
 			}
 		} else if isSingle && len(trialsResp.Trials) > 0 {
 			if _, err := tx.NewUpdate().Model(&model.Trial{}).
@@ -1795,7 +1795,7 @@ func (a *apiServer) ExpMetricNames(req *apiv1.ExpMetricNamesRequest,
 	var timeSinceLastAuth time.Time
 	for {
 		var response apiv1.ExpMetricNamesResponse
-		if time.Now().Sub(timeSinceLastAuth) >= recheckAuthPeriod {
+		if time.Since(timeSinceLastAuth) >= recheckAuthPeriod {
 			for _, expID := range req.Ids {
 				exp, _, err := a.getExperimentAndCheckCanDoActions(resp.Context(), int(expID),
 					experiment.AuthZProvider.Get().CanGetExperimentArtifacts)
@@ -1895,7 +1895,7 @@ func (a *apiServer) MetricBatches(req *apiv1.MetricBatchesRequest,
 	seenBatches := make(map[int32]bool)
 	var startTime time.Time
 	for {
-		if time.Now().Sub(timeSinceLastAuth) >= recheckAuthPeriod {
+		if time.Since(timeSinceLastAuth) >= recheckAuthPeriod {
 			if _, _, err := a.getExperimentAndCheckCanDoActions(resp.Context(), experimentID,
 				experiment.AuthZProvider.Get().CanGetExperimentArtifacts); err != nil {
 				return err
@@ -1996,7 +1996,7 @@ func (a *apiServer) TrialsSnapshot(req *apiv1.TrialsSnapshotRequest,
 	var timeSinceLastAuth time.Time
 	var startTime time.Time
 	for {
-		if time.Now().Sub(timeSinceLastAuth) >= recheckAuthPeriod {
+		if time.Since(timeSinceLastAuth) >= recheckAuthPeriod {
 			if _, _, err := a.getExperimentAndCheckCanDoActions(resp.Context(), experimentID,
 				experiment.AuthZProvider.Get().CanGetExperimentArtifacts); err != nil {
 				return err
@@ -2184,7 +2184,7 @@ func (a *apiServer) TrialsSample(req *apiv1.TrialsSampleRequest,
 	trialCursors := make(map[int32]time.Time)
 	currentTrials := make(map[int32]bool)
 	for {
-		if time.Now().Sub(timeSinceLastAuth) >= recheckAuthPeriod {
+		if time.Since(timeSinceLastAuth) >= recheckAuthPeriod {
 			exp, _, err := a.getExperimentAndCheckCanDoActions(resp.Context(), experimentID,
 				experiment.AuthZProvider.Get().CanGetExperimentArtifacts)
 			if err != nil {

master/internal/api_experiment_intg_test.go

@@ -53,6 +53,7 @@ import (
 	"github.com/determined-ai/determined/proto/pkg/workspacev1"
 )
 
+//nolint:containedctx
 type mockStream[T any] struct {
 	mu   sync.Mutex
 	ctx  context.Context
@@ -123,7 +124,7 @@ func minExpConfToYaml(t *testing.T) string {
 	return string(bytes)
 }
 
-// nolint: exhaustivestruct
+// nolint: exhaustruct
 var minExpConfig = expconf.ExperimentConfig{
 	RawResources: &expconf.ResourcesConfig{
 		RawResourcePool: ptrs.Ptr("kubernetes"),
@@ -337,7 +338,7 @@ searcher:
 		"override config is provided and experiment is not single searcher, got 'random' instead")
 }
 
-// nolint: exhaustivestruct
+// nolint: exhaustruct
 func TestCreateExperimentCheckpointStorage(t *testing.T) {
 	api, _, ctx := setupAPITest(t, nil)
 	api.m.config.CheckpointStorage = expconf.CheckpointStorageConfig{}
@@ -437,7 +438,7 @@ checkpoint_storage:
 	require.Equal(t, expected, resp.Config.AsMap()["checkpoint_storage"])
 }
 
-// nolint: exhaustivestruct
+// nolint: exhaustruct
 func TestGetExperiments(t *testing.T) {
 	// Setup.
 	api, _, ctx := setupAPITest(t, nil)
@@ -863,7 +864,7 @@ func TestLegacyExperiments(t *testing.T) {
 
 var res *apiv1.GetExperimentsResponse // Avoid compiler optimizing res out.
 
-// nolint: exhaustivestruct
+// nolint: exhaustruct
 func benchmarkGetExperiments(b *testing.B, n int) {
 	// This should be fine as long as no error happens. For some
 	// reason passing nil gives an error. In addition this
@@ -942,7 +943,7 @@ func BenchmarkGetExeriments500(b *testing.B) { benchmarkGetExperiments(b, 500) }
 
 func BenchmarkGetExeriments2500(b *testing.B) { benchmarkGetExperiments(b, 2500) }
 
-// nolint: exhaustivestruct
+// nolint: exhaustruct
 func createTestExpWithProjectID(
 	t *testing.T, api *apiServer, curUser model.User, projectID int, labels ...string,
 ) *model.Experiment {

master/internal/api_model.go

@@ -365,7 +365,7 @@ func (a *apiServer) PatchModel(
 	if req.Model.Labels != nil {
 		// avoid duplicate keys
 		reqLabelSet := make(map[string]struct{}, len(req.Model.Labels.Values))
-		reqLabelList := make([]string, len(reqLabelSet))
+		reqLabelList := make([]string, 0, len(reqLabelSet))
 		for _, el := range req.Model.Labels.Values {
 			if _, ok := el.GetKind().(*structpb.Value_StringValue); !ok {
 				// Invalid label.

master/internal/api_project.go

@@ -275,7 +275,7 @@ func (a *apiServer) getProjectColumnsByID(
 		flatHparam := expconf.FlattenHPs(hparam.Hyperparameters)
 
 		// ensure we're iterating in order
-		paramKeys := make([]string, len(flatHparam))
+		paramKeys := make([]string, 0, len(flatHparam))
 		for key := range flatHparam {
 			paramKeys = append(paramKeys, key)
 		}