Enable errcheck everywhere and fix or silent remaining issues (#1987)

## Changes
Enable errcheck linter for the whole codebase.

Fix remaining complaints:
- If we can propagate error to caller, do that
- If we writing to stdout, continue ignoring errors (to avoid crashing
in "cli | head" case)
- Add exception for cobra non-critical API such as
MarkHidden/MarkDeprecated/RegisterFlagCompletionFunc. This keeps current
code and behaviour, to be decided later if we want to change this.
- Continue ignoring errors where that is desired behaviour (e.g.
git.loadConfig).
- Continue ignoring errors where panicking seems riskier than ignoring
the error.
- Annotate cases in libs/dyn with //nolint:errcheck - to be addressed
later.

Note, this PR is not meant to come up with the best strategy for each
case, but to be a relative safe change to enable errcheck linter.
  
## Tests
Existing tests.

.golangci.yaml

@@ -16,9 +16,11 @@ linters-settings:
         replacement: 'a[b:]'
       - pattern: 'interface{}'
         replacement: 'any'
+  errcheck:
+    exclude-functions:
+      - (*github.com/spf13/cobra.Command).RegisterFlagCompletionFunc
+      - (*github.com/spf13/cobra.Command).MarkFlagRequired
+      - (*github.com/spf13/pflag.FlagSet).MarkDeprecated
+      - (*github.com/spf13/pflag.FlagSet).MarkHidden
 issues:
   exclude-dirs-use-default: false  # recommended by docs https://golangci-lint.run/usage/false-positives/
-  exclude-rules:
-    - path-except: (_test\.go|internal)
-      linters:
-        - errcheck

bundle/config/mutator/initialize_urls.go

@@ -32,7 +32,10 @@ func (m *initializeURLs) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagn
 	}
 	orgId := strconv.FormatInt(workspaceId, 10)
 	host := b.WorkspaceClient().Config.CanonicalHostName()
-	initializeForWorkspace(b, orgId, host)
+	err = initializeForWorkspace(b, orgId, host)
+	if err != nil {
+		return diag.FromErr(err)
+	}
 	return nil
 }
 

bundle/config/mutator/resolve_resource_references.go

@@ -36,8 +36,7 @@ func (m *resolveResourceReferences) Apply(ctx context.Context, b *bundle.Bundle)
 				return fmt.Errorf("failed to resolve %s, err: %w", v.Lookup, err)
 			}
 
-			v.Set(id)
-			return nil
+			return v.Set(id)
 		})
 	}
 

bundle/deploy/state_pull.go

@@ -62,8 +62,14 @@ func (s *statePull) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostic
 	}
 
 	// Truncating the file before writing
-	local.Truncate(0)
-	local.Seek(0, 0)
+	err = local.Truncate(0)
+	if err != nil {
+		return diag.FromErr(err)
+	}
+	_, err = local.Seek(0, 0)
+	if err != nil {
+		return diag.FromErr(err)
+	}
 
 	// Write file to disk.
 	log.Infof(ctx, "Writing remote deployment state file to local cache directory")

bundle/render/render_text_output.go

@@ -171,10 +171,16 @@ func RenderDiagnostics(out io.Writer, b *bundle.Bundle, diags diag.Diagnostics,
 			if err != nil {
 				return fmt.Errorf("failed to render summary: %w", err)
 			}
-			io.WriteString(out, "\n")
+			_, err = io.WriteString(out, "\n")
+			if err != nil {
+				return err
+			}
 		}
 		trailer := buildTrailer(diags)
-		io.WriteString(out, trailer)
+		_, err = io.WriteString(out, trailer)
+		if err != nil {
+			return err
+		}
 	}
 
 	return nil

cmd/auth/describe.go

@@ -141,7 +141,10 @@ func render(ctx context.Context, cmd *cobra.Command, status *authStatus, templat
 		if err != nil {
 			return err
 		}
-		cmd.OutOrStdout().Write(buf)
+		_, err = cmd.OutOrStdout().Write(buf)
+		if err != nil {
+			return err
+		}
 	default:
 		return fmt.Errorf("unknown output type %s", root.OutputType(cmd))
 	}

cmd/auth/env.go

@@ -138,7 +138,7 @@ func newEnvCommand() *cobra.Command {
 		if err != nil {
 			return err
 		}
-		cmd.OutOrStdout().Write(raw)
+		_, _ = cmd.OutOrStdout().Write(raw)
 		return nil
 	}
 

cmd/auth/token.go

@@ -94,7 +94,7 @@ func newTokenCommand(persistentAuth *auth.PersistentAuth) *cobra.Command {
 		if err != nil {
 			return err
 		}
-		cmd.OutOrStdout().Write(raw)
+		_, _ = cmd.OutOrStdout().Write(raw)
 		return nil
 	}
 

cmd/bundle/debug/terraform.go

@@ -60,13 +60,13 @@ For more information about filesystem mirrors, see the Terraform documentation:
 		}
 		switch root.OutputType(cmd) {
 		case flags.OutputText:
-			cmdio.Render(cmd.Context(), dependencies.Terraform)
+			_ = cmdio.Render(cmd.Context(), dependencies.Terraform)
 		case flags.OutputJSON:
 			buf, err := json.MarshalIndent(dependencies, "", "  ")
 			if err != nil {
 				return err
 			}
-			cmd.OutOrStdout().Write(buf)
+			_, _ = cmd.OutOrStdout().Write(buf)
 		default:
 			return fmt.Errorf("unknown output type %s", root.OutputType(cmd))
 		}

cmd/bundle/run.go

@@ -159,13 +159,19 @@ task or a Python wheel task, the second example applies.
 				if err != nil {
 					return err
 				}
-				cmd.OutOrStdout().Write([]byte(resultString))
+				_, err = cmd.OutOrStdout().Write([]byte(resultString))
+				if err != nil {
+					return err
+				}
 			case flags.OutputJSON:
 				b, err := json.MarshalIndent(output, "", "  ")
 				if err != nil {
 					return err
 				}
-				cmd.OutOrStdout().Write(b)
+				_, err = cmd.OutOrStdout().Write(b)
+				if err != nil {
+					return err
+				}
 			default:
 				return fmt.Errorf("unknown output type %s", root.OutputType(cmd))
 			}

cmd/bundle/summary.go

@@ -73,7 +73,7 @@ func newSummaryCommand() *cobra.Command {
 			if err != nil {
 				return err
 			}
-			cmd.OutOrStdout().Write(buf)
+			_, _ = cmd.OutOrStdout().Write(buf)
 		default:
 			return fmt.Errorf("unknown output type %s", root.OutputType(cmd))
 		}

cmd/bundle/validate.go

@@ -20,7 +20,7 @@ func renderJsonOutput(cmd *cobra.Command, b *bundle.Bundle, diags diag.Diagnosti
 	if err != nil {
 		return err
 	}
-	cmd.OutOrStdout().Write(buf)
+	_, _ = cmd.OutOrStdout().Write(buf)
 	return diags.Error()
 }
 

cmd/root/io.go

@@ -21,7 +21,7 @@ func initOutputFlag(cmd *cobra.Command) *outputFlag {
 	// Configure defaults from environment, if applicable.
 	// If the provided value is invalid it is ignored.
 	if v, ok := env.Lookup(cmd.Context(), envOutputFormat); ok {
-		f.output.Set(v)
+		f.output.Set(v) //nolint:errcheck
 	}
 
 	cmd.PersistentFlags().VarP(&f.output, "output", "o", "output type: text or json")

cmd/root/logger.go

@@ -45,7 +45,10 @@ func (f *logFlags) makeLogHandler(opts slog.HandlerOptions) (slog.Handler, error
 
 func (f *logFlags) initializeContext(ctx context.Context) (context.Context, error) {
 	if f.debug {
-		f.level.Set("debug")
+		err := f.level.Set("debug")
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	opts := slog.HandlerOptions{}
@@ -81,13 +84,13 @@ func initLogFlags(cmd *cobra.Command) *logFlags {
 	// Configure defaults from environment, if applicable.
 	// If the provided value is invalid it is ignored.
 	if v, ok := env.Lookup(cmd.Context(), envLogFile); ok {
-		f.file.Set(v)
+		f.file.Set(v) //nolint:errcheck
 	}
 	if v, ok := env.Lookup(cmd.Context(), envLogLevel); ok {
-		f.level.Set(v)
+		f.level.Set(v) //nolint:errcheck
 	}
 	if v, ok := env.Lookup(cmd.Context(), envLogFormat); ok {
-		f.output.Set(v)
+		f.output.Set(v) //nolint:errcheck
 	}
 
 	flags := cmd.PersistentFlags()

cmd/root/progress_logger.go

@@ -59,7 +59,7 @@ func initProgressLoggerFlag(cmd *cobra.Command, logFlags *logFlags) *progressLog
 	// Configure defaults from environment, if applicable.
 	// If the provided value is invalid it is ignored.
 	if v, ok := env.Lookup(cmd.Context(), envProgressFormat); ok {
-		f.Set(v)
+		_ = f.Set(v)
 	}
 
 	flags := cmd.PersistentFlags()

libs/auth/callback.go

@@ -53,7 +53,9 @@ func newCallback(ctx context.Context, a *PersistentAuth) (*callbackServer, error
 		a:           a,
 	}
 	cb.srv.Handler = cb
-	go cb.srv.Serve(cb.ln)
+	go func() {
+		_ = cb.srv.Serve(cb.ln)
+	}()
 	return cb, nil
 }
 

libs/cmdio/logger.go

@@ -188,29 +188,29 @@ func (l *Logger) writeJson(event Event) {
 		// we panic because there we cannot catch this in jobs.RunNowAndWait
 		panic(err)
 	}
-	l.Writer.Write([]byte(b))
-	l.Writer.Write([]byte("\n"))
+	_, _ = l.Writer.Write([]byte(b))
+	_, _ = l.Writer.Write([]byte("\n"))
 }
 
 func (l *Logger) writeAppend(event Event) {
-	l.Writer.Write([]byte(event.String()))
-	l.Writer.Write([]byte("\n"))
+	_, _ = l.Writer.Write([]byte(event.String()))
+	_, _ = l.Writer.Write([]byte("\n"))
 }
 
 func (l *Logger) writeInplace(event Event) {
 	if l.isFirstEvent {
 		// save cursor location
-		l.Writer.Write([]byte("\033[s"))
+		_, _ = l.Writer.Write([]byte("\033[s"))
 	}
 
 	// move cursor to saved location
-	l.Writer.Write([]byte("\033[u"))
+	_, _ = l.Writer.Write([]byte("\033[u"))
 
 	// clear from cursor to end of screen
-	l.Writer.Write([]byte("\033[0J"))
+	_, _ = l.Writer.Write([]byte("\033[0J"))
 
-	l.Writer.Write([]byte(event.String()))
-	l.Writer.Write([]byte("\n"))
+	_, _ = l.Writer.Write([]byte(event.String()))
+	_, _ = l.Writer.Write([]byte("\n"))
 	l.isFirstEvent = false
 }
 

libs/cmdio/render.go

@@ -361,7 +361,9 @@ func renderUsingTemplate(ctx context.Context, r templateRenderer, w io.Writer, h
 		if err != nil {
 			return err
 		}
-		tw.Write([]byte("\n"))
+		if _, err := tw.Write([]byte("\n")); err != nil {
+			return err
+		}
 		// Do not flush here. Instead, allow the first 100 resources to determine the initial spacing of the header columns.
 	}
 	t, err := base.Parse(tmpl)

libs/dyn/convert/from_typed.go

@@ -126,7 +126,7 @@ func fromTypedStruct(src reflect.Value, ref dyn.Value, options ...fromTypedOptio
 
 		// Either if the key was set in the reference or the field is not zero-valued, we include it.
 		if ok || nv.Kind() != dyn.KindNil {
-			out.Set(refk, nv)
+			out.Set(refk, nv) // nolint:errcheck
 		}
 	}
 
@@ -184,7 +184,7 @@ func fromTypedMap(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
 
 		// Every entry is represented, even if it is a nil.
 		// Otherwise, a map with zero-valued structs would yield a nil as well.
-		out.Set(refk, nv)
+		out.Set(refk, nv) //nolint:errcheck
 	}
 
 	return dyn.V(out), nil

libs/dyn/convert/normalize.go

@@ -116,7 +116,7 @@ func (n normalizeOptions) normalizeStruct(typ reflect.Type, src dyn.Value, seen
 				}
 			}
 
-			out.Set(pk, nv)
+			out.Set(pk, nv) //nolint:errcheck
 		}
 
 		// Return the normalized value if missing fields are not included.
@@ -162,7 +162,7 @@ func (n normalizeOptions) normalizeStruct(typ reflect.Type, src dyn.Value, seen
 				continue
 			}
 			if v.IsValid() {
-				out.Set(dyn.V(k), v)
+				out.Set(dyn.V(k), v) // nolint:errcheck
 			}
 		}
 
@@ -201,7 +201,7 @@ func (n normalizeOptions) normalizeMap(typ reflect.Type, src dyn.Value, seen []r
 				}
 			}
 
-			out.Set(pk, nv)
+			out.Set(pk, nv) //nolint:errcheck
 		}
 
 		return dyn.NewValue(out, src.Locations()), diags

libs/dyn/jsonloader/json.go

@@ -70,7 +70,7 @@ func decodeValue(decoder *json.Decoder, o *Offset) (dyn.Value, error) {
 					return invalidValueWithLocation(decoder, o), err
 				}
 
-				obj.Set(keyVal, val)
+				obj.Set(keyVal, val) //nolint:errcheck
 			}
 			// Consume the closing '}'
 			if _, err := decoder.Token(); err != nil {

libs/dyn/mapping.go

@@ -41,7 +41,7 @@ func newMappingWithSize(size int) Mapping {
 func newMappingFromGoMap(vin map[string]Value) Mapping {
 	m := newMappingWithSize(len(vin))
 	for k, v := range vin {
-		m.Set(V(k), v)
+		m.Set(V(k), v) //nolint:errcheck
 	}
 	return m
 }
@@ -144,6 +144,6 @@ func (m Mapping) Clone() Mapping {
 // Merge merges the key-value pairs from another Mapping into the current Mapping.
 func (m *Mapping) Merge(n Mapping) {
 	for _, p := range n.pairs {
-		m.Set(p.Key, p.Value)
+		m.Set(p.Key, p.Value) //nolint:errcheck
 	}
 }

libs/dyn/merge/merge.go

@@ -88,10 +88,10 @@ func mergeMap(a, b dyn.Value) (dyn.Value, error) {
 			if err != nil {
 				return dyn.InvalidValue, err
 			}
-			out.Set(pk, merged)
+			out.Set(pk, merged) //nolint:errcheck
 		} else {
 			// Otherwise, just set the value.
-			out.Set(pk, pv)
+			out.Set(pk, pv) //nolint:errcheck
 		}
 	}
 

libs/dyn/pattern.go

@@ -69,7 +69,7 @@ func (c anyKeyComponent) visit(v Value, prefix Path, suffix Pattern, opts visitO
 			return InvalidValue, err
 		}
 
-		m.Set(pk, nv)
+		m.Set(pk, nv) //nolint:errcheck
 	}
 
 	return NewValue(m, v.Locations()), nil

libs/dyn/visit.go

@@ -122,7 +122,7 @@ func (component pathComponent) visit(v Value, prefix Path, suffix Pattern, opts
 
 		// Return an updated map value.
 		m = m.Clone()
-		m.Set(V(component.key), nv)
+		m.Set(V(component.key), nv) //nolint:errcheck
 		return Value{
 			v: m,
 			k: KindMap,

libs/dyn/visit_map.go

@@ -25,7 +25,7 @@ func Foreach(fn MapFunc) MapFunc {
 				if err != nil {
 					return InvalidValue, err
 				}
-				m.Set(pk, nv)
+				m.Set(pk, nv) //nolint:errcheck
 			}
 			return NewValue(m, v.Locations()), nil
 		case KindSequence:

libs/dyn/visit_set.go

@@ -41,7 +41,7 @@ func SetByPath(v Value, p Path, nv Value) (Value, error) {
 
 				// Return an updated map value.
 				m = m.Clone()
-				m.Set(V(component.key), nv)
+				m.Set(V(component.key), nv) //nolint:errcheck
 				return Value{
 					v: m,
 					k: KindMap,

libs/dyn/walk.go

@@ -45,7 +45,7 @@ func walk(v Value, p Path, fn func(p Path, v Value) (Value, error)) (Value, erro
 			if err != nil {
 				return InvalidValue, err
 			}
-			out.Set(pk, nv)
+			out.Set(pk, nv) //nolint:errcheck
 		}
 		v.v = out
 	case KindSequence:

libs/dyn/yamlloader/loader.go

@@ -129,7 +129,7 @@ func (d *loader) loadMapping(node *yaml.Node, loc dyn.Location) (dyn.Value, erro
 			return dyn.InvalidValue, err
 		}
 
-		acc.Set(k, v)
+		acc.Set(k, v) //nolint:errcheck
 	}
 
 	if merge == nil {

libs/env/loader.go

@@ -43,7 +43,9 @@ func (le *configLoader) Configure(cfg *config.Config) error {
 			if v == "" {
 				continue
 			}
-			a.Set(cfg, v)
+			if err := a.Set(cfg, v); err != nil {
+				return err
+			}
 		}
 	}
 	return nil

libs/git/config.go

@@ -155,8 +155,8 @@ func globalGitConfig() (*config, error) {
 	// > are missing or unreadable they will be ignored.
 	//
 	// We therefore ignore the error return value for the calls below.
-	config.loadFile(vfs.MustNew(xdgConfigHome), "git/config")
-	config.loadFile(vfs.MustNew(config.home), ".gitconfig")
+	_ = config.loadFile(vfs.MustNew(xdgConfigHome), "git/config")
+	_ = config.loadFile(vfs.MustNew(config.home), ".gitconfig")
 
 	return config, nil
 }

libs/process/stub.go

@@ -148,13 +148,20 @@ func (s *processStub) run(cmd *exec.Cmd) error {
 		if !re.MatchString(norm) {
 			continue
 		}
+		err := resp.err
 		if resp.stdout != "" {
-			cmd.Stdout.Write([]byte(resp.stdout))
+			_, err1 := cmd.Stdout.Write([]byte(resp.stdout))
+			if err == nil {
+				err = err1
+			}
 		}
 		if resp.stderr != "" {
-			cmd.Stderr.Write([]byte(resp.stderr))
+			_, err1 := cmd.Stderr.Write([]byte(resp.stderr))
+			if err == nil {
+				err = err1
+			}
 		}
-		return resp.err
+		return err
 	}
 	if s.callback != nil {
 		return s.callback(cmd)
@@ -163,8 +170,12 @@ func (s *processStub) run(cmd *exec.Cmd) error {
 	if s.reponseStub == zeroStub {
 		return fmt.Errorf("no default process stub")
 	}
+	err := s.reponseStub.err
 	if s.reponseStub.stdout != "" {
-		cmd.Stdout.Write([]byte(s.reponseStub.stdout))
+		_, err1 := cmd.Stdout.Write([]byte(s.reponseStub.stdout))
+		if err == nil {
+			err = err1
+		}
 	}
-	return s.reponseStub.err
+	return err
 }

libs/sync/output.go

@@ -43,9 +43,9 @@ func TextOutput(ctx context.Context, ch <-chan Event, w io.Writer) {
 			// Log only if something actually happened.
 			// Sync events produce an empty string if nothing happened.
 			if str := e.String(); str != "" {
-				bw.WriteString(str)
-				bw.WriteString("\n")
-				bw.Flush()
+				_, _ = bw.WriteString(str)
+				_, _ = bw.WriteString("\n")
+				_ = bw.Flush()
 			}
 		}
 	}