Cosmetic cleanup and linting

This is mostly a cosmetic changeset:
- refined golangcilint, explicitly calling which linters we really care about, along with settings for revive
- comments on reasons for some of //nolint
- comments line breaks
- additional FIXME information
- assert type check fixes
- const-ification
- overall making linter happy(-ier)

Signed-off-by: apostasie <[email protected]>

Author: apostasie

mod/tigron/.golangci.yml

@@ -12,25 +12,80 @@ issues:
 
 linters:
   default: all
+  enable:
+    # These are the default set of golangci (errcheck is disabled, see below)
+    - govet             # Vet examines Go source code and reports suspicious constructs. It is roughly the same as 'go vet' and uses its passes.
+    - ineffassign       # Detects when assignments to existing variables are not used.
+    - staticcheck       # It's the set of rules from staticcheck.
+    - unused            # Checks Go code for unused constants, variables, functions and types.
+    # These are the linters we knowingly want enabled in addition to the default set
+    - containedctx      # avoid embedding context into structs
+    - depguard          # Allows to explicitly allow or disallow third party modules
+    - err113            # encourage static errors
+    - gochecknoglobals  # globals should be avoided as much as possible
+    - godot             # forces dot at the end of comments
+    - gosec             # various security checks
+    - interfacebloat    # limit complexity in public APIs
+    - paralleltest      # enforces tests using parallel
+    - revive            # meta linter (see settings below)
+    - testpackage       # test packages should be separate from the package they test (eg: name them package_test)
+    - testableexamples  # makes sure that examples are testable (have an expected output)
+    - thelper           # enforces use of t.Helper()
+    - varnamelen        # encourage readable descriptive names for variables instead of x, y, z
   disable:
     # These are the linters that we know we do not want
-    - cyclop          # provided by revive
-    - exhaustruct     # does not serve much of a purpose
-    - funlen          # provided by revive
-    - gocognit        # provided by revive
-    - goconst         # provided by revive
-    - godox           # not helpful unless we could downgrade it to warning / info
-    - ginkgolinter    # no ginkgo
-    - gomodguard      # we use depguard instead
-    - ireturn         # too annoying with not enough value
-    - lll             # provided by golines
-    - nonamedreturns  # named returns are occasionally useful
-    - prealloc        # premature optimization
-    - promlinter      # no prometheus
-    - sloglint        # no slog
-    - testifylint     # no testify
-    - zerologlint     # no zerolog
+    - cyclop            # provided by revive
+    - exhaustruct       # does not serve much of a purpose
+    - errcheck          # provided by revive
+    - forcetypeassert   # provided by revive
+    - funlen            # provided by revive
+    - gocognit          # provided by revive
+    - goconst           # provided by revive
+    - godox             # not helpful unless we could downgrade it to warning / info
+    - ginkgolinter      # no ginkgo
+    - gomodguard        # we use depguard instead
+    - ireturn           # too annoying with not enough value
+    - lll               # provided by golines
+    - nonamedreturns    # named returns are occasionally useful
+    - prealloc          # premature optimization
+    - promlinter        # no prometheus
+    - sloglint          # no slog
+    - testifylint       # no testify
+    - zerologlint       # no zerolog
   settings:
+    interfacebloat:
+      # Default is 10
+      max: 13
+    revive:
+      enable-all-rules: true
+      rules:
+        - name: cognitive-complexity
+          # Default is 7
+          arguments: [60]
+        - name: function-length
+          # Default is 50, 75
+          arguments: [80, 160]
+        - name: cyclomatic
+          # Default is 10
+          arguments: [30]
+        - name: add-constant
+          arguments:
+            - allowInts: "0,1,2"
+              allowStrs: '""'
+        - name: flag-parameter
+          # Not sure why this is valuable.
+          disabled: true
+        - name: line-length-limit
+          # Formatter `golines` takes care of this.
+          disabled: true
+        - name: unhandled-error
+          arguments:
+            - "fmt.Print"
+            - "fmt.Println"
+            - "fmt.Printf"
+            - "fmt.Fprint"
+            - "fmt.Fprintln"
+            - "fmt.Fprintf"
     depguard:
       rules:
         main:

mod/tigron/expect/comparators_test.go

@@ -14,6 +14,7 @@
    limitations under the License.
 */
 
+//revive:disable:add-constant
 package expect_test
 
 import (

mod/tigron/internal/com/command.go

@@ -43,16 +43,14 @@ var (
 	ErrFailedStarting = errors.New("command failed starting")
 	// ErrSignaled is returned by Wait() if a signal was sent to the command while running.
 	ErrSignaled = errors.New("command execution signaled")
-	// ErrExecutionFailed is returned by Wait() when a command executes but returns a non-zero error
-	// code.
+	// ErrExecutionFailed is returned by Wait() when a command executes but returns a non-zero error code.
 	ErrExecutionFailed = errors.New("command returned a non-zero exit code")
 	// ErrFailedSendingSignal may happen if sending a signal to an already terminated process.
 	ErrFailedSendingSignal = errors.New("failed sending signal")
 
 	// ErrExecAlreadyStarted is a system error normally indicating a bogus double call to Run().
 	ErrExecAlreadyStarted = errors.New("command has already been started (double `Run`)")
-	// ErrExecNotStarted is a system error normally indicating that Wait() has been called without
-	// first calling Run().
+	// ErrExecNotStarted is a system error normally indicating that Wait() has been called without first calling Run().
 	ErrExecNotStarted = errors.New("command has not been started (call `Run` first)")
 	// ErrExecAlreadyFinished is a system error indicating a double call to Wait().
 	ErrExecAlreadyFinished = errors.New("command is already finished")
@@ -75,7 +73,7 @@ type Result struct {
 }
 
 type execution struct {
-	//nolint:containedctx
+	//nolint:containedctx // Is there a way around this?
 	context context.Context
 	cancel  context.CancelFunc
 	command *exec.Cmd
@@ -138,27 +136,24 @@ func (gc *Command) Clone() *Command {
 	return com
 }
 
-// WithPTY requests that the command be executed with a pty for std streams. Parameters allow
-// showing which streams
-// are to be tied to the pty.
+// WithPTY requests that the command be executed with a pty for std streams.
+// Parameters allow showing which streams are to be tied to the pty.
 // This command has no effect if Run has already been called.
 func (gc *Command) WithPTY(stdin, stdout, stderr bool) {
 	gc.ptyStdout = stdout
 	gc.ptyStderr = stderr
 	gc.ptyStdin = stdin
 }
 
-// WithFeeder ensures that the provider function will be executed and its output fed to the command
-// stdin. WithFeeder, like Feed, can be used multiple times, and writes will be performed
-// sequentially, in order.
+// WithFeeder ensures that the provider function will be executed and its output fed to the command stdin.
+// WithFeeder, like Feed, can be used multiple times, and writes will be performed sequentially, in order.
 // This command has no effect if Run has already been called.
 func (gc *Command) WithFeeder(writers ...func() io.Reader) {
 	gc.writers = append(gc.writers, writers...)
 }
 
 // Feed ensures that the provider reader will be copied on the command stdin.
-// Feed, like WithFeeder, can be used multiple times, and writes will be performed in sequentially,
-// in order.
+// Feed, like WithFeeder, can be used multiple times, and writes will be performed in sequentially, in order.
 // This command has no effect if Run has already been called.
 func (gc *Command) Feed(reader io.Reader) {
 	gc.writers = append(gc.writers, func() io.Reader {
@@ -198,7 +193,6 @@ func (gc *Command) Run(parentCtx context.Context) error {
 
 	// Create a contextual command, set the logger
 	cmd = gc.buildCommand(ctx)
-
 	// Get a debug-logger from the context
 	var (
 		log logger.Logger
@@ -339,8 +333,7 @@ func (gc *Command) wrap() error {
 		err      error
 	)
 
-	// XXXgolang: this is troubling. cmd.ProcessState.ExitCode() is always fine, even if
-	// cmd.ProcessState is nil.
+	// XXXgolang: this is troubling. cmd.ProcessState.ExitCode() is always fine, even if cmd.ProcessState is nil.
 	exitCode = cmd.ProcessState.ExitCode()
 
 	if cmd.ProcessState != nil {
@@ -357,7 +350,7 @@ func (gc *Command) wrap() error {
 		}
 	}
 
-	// Catch-up on the context
+	// Catch-up on the context.
 	switch ctx.Err() {
 	case context.DeadlineExceeded:
 		err = ErrTimeout
@@ -366,7 +359,7 @@ func (gc *Command) wrap() error {
 	default:
 	}
 
-	// Stuff everything in Result and return err
+	// Stuff everything in Result and return err.
 	gc.result = &Result{
 		ExitCode: exitCode,
 		Stdout:   pipes.fromStdout,
@@ -383,7 +376,7 @@ func (gc *Command) wrap() error {
 }
 
 func (gc *Command) buildCommand(ctx context.Context) *exec.Cmd {
-	// Build arguments and binary
+	// Build arguments and binary.
 	args := gc.Args
 	if gc.PrependArgs != nil {
 		args = append(gc.PrependArgs, args...)
@@ -459,12 +452,12 @@ func (gc *Command) buildCommand(ctx context.Context) *exec.Cmd {
 		cmd.Env = append(cmd.Env, k+"="+v)
 	}
 
-	// Attach platform ProcAttr and get optional custom cancellation routine
+	// Attach platform ProcAttr and get optional custom cancellation routine.
 	if cancellation := addAttr(cmd); cancellation != nil {
 		cmd.Cancel = func() error {
 			gc.exec.log.Log("command cancelled")
 
-			// Call the platform dependent cancellation routine
+			// Call the platform dependent cancellation routine.
 			return cancellation()
 		}
 	}

mod/tigron/internal/com/command_other.go

@@ -24,10 +24,10 @@ import (
 )
 
 func addAttr(cmd *exec.Cmd) func() error {
-	// Default shutdown will leave child processes behind in certain circumstances
+	// Default shutdown will leave child processes behind in certain circumstances.
 	cmd.SysProcAttr = &syscall.SysProcAttr{
 		Setsid: true,
-		// FIXME: understand why we would want that
+		// FIXME: understand why we would want that.
 		// Setctty: true,
 	}
 

mod/tigron/internal/com/package_example_test.go

@@ -14,6 +14,7 @@
    limitations under the License.
 */
 
+//revive:disable:add-constant
 package com_test
 
 import (

mod/tigron/internal/com/package_test.go

@@ -50,19 +50,19 @@ func TestMain(m *testing.M) {
 		diff := highk.Diff(string(before), string(after))
 
 		if len(diff) != 0 {
-			_, _ = fmt.Fprintln(os.Stderr, "Leaking file descriptors")
+			fmt.Fprintln(os.Stderr, "Leaking file descriptors")
 
 			for _, file := range diff {
-				_, _ = fmt.Fprintln(os.Stderr, file)
+				fmt.Fprintln(os.Stderr, file)
 			}
 
 			exitCode = 1
 		}
 	}
 
 	if err := highk.FindGoRoutines(); err != nil {
-		_, _ = fmt.Fprintln(os.Stderr, "Leaking go routines")
-		_, _ = fmt.Fprintln(os.Stderr, err.Error())
+		fmt.Fprintln(os.Stderr, "Leaking go routines")
+		fmt.Fprintln(os.Stderr, err.Error())
 
 		exitCode = 1
 	}

mod/tigron/internal/com/pipes.go

@@ -33,11 +33,9 @@ import (
 var (
 	// ErrFailedCreating could be returned by newStdPipes() on pty creation failure.
 	ErrFailedCreating = errors.New("failed acquiring pipe")
-	// ErrFailedReading could be returned by the ioGroup in case the go routines fails to read out
-	// of a pipe.
+	// ErrFailedReading could be returned by the ioGroup in case the go routines fails to read out of a pipe.
 	ErrFailedReading = errors.New("failed reading")
-	// ErrFailedWriting could be returned by the ioGroup in case the go routines fails to write on a
-	// pipe.
+	// ErrFailedWriting could be returned by the ioGroup in case the go routines fails to write on a pipe.
 	ErrFailedWriting = errors.New("failed writing")
 )
 
@@ -108,7 +106,6 @@ func (pipes *stdPipes) closeCaller() {
 	}
 }
 
-//nolint:gocognit
 func newStdPipes(
 	ctx context.Context,
 	log *logger.ConcreteLogger,

mod/tigron/internal/doc.go

@@ -0,0 +1,21 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+// Package internal provides an assert library, pty, a command wrapper, and a leak detection library
+// for internal use in Tigron. The objective for these is not to become generic use-cases libraries,
+// but instead to deliver what Tigron
+// needs in the simplest possible form.
+package internal

mod/tigron/internal/exit.go

@@ -14,11 +14,6 @@
    limitations under the License.
 */
 
-// Package internal provides an assert library, pty, a command wrapper, and a leak detection library
-// for internal use in Tigron.
-// The objective for these is not to become generic use-cases libraries, but instead to deliver what
-// Tigron needs
-// in the simplest possible form.
 package internal
 
 // This is duplicated from `expect` to avoid circular imports.

mod/tigron/internal/formatter/osc8.go

@@ -27,5 +27,6 @@ type OSC8 struct {
 
 func (o *OSC8) String() string {
 	// FIXME: not sure if any desktop software does support line numbers anchors?
+	// FIXME: test that the terminal is able to display these and fallback to printing the information if not.
 	return fmt.Sprintf("\x1b]8;;%s#%d:1\x07%s\x1b]8;;\x07"+"\u001b[0m", o.Location, o.Line, o.Text)
 }

mod/tigron/internal/highk/doc.go

@@ -14,8 +14,8 @@
    limitations under the License.
 */
 
-// Package highk (for "high-κ dielectric") is a highly experimental leak detection library
-// (for file descriptors and go routines).
+// Package highk (for "high-κ dielectric") is a highly experimental leak detection library (for file descriptors and go
+// routines).
 // It is purely internal for now and used only as part of the tests for tigron.
 // TODO:
 // - get rid of lsof and implement in go

mod/tigron/internal/highk/fileleak.go

@@ -26,20 +26,20 @@ import (
 	"syscall"
 )
 
-// FIXME: it seems that lsof (or go test) is interefering and showing false positive KQUEUE / inodes
+// FIXME: it seems that lsof (or go test) is interfering and showing false positive KQUEUE / inodes
 //
-//nolint:gochecknoglobals
+//nolint:gochecknoglobals // FIXME rewrite all of this anyhow
 var whitelist = map[string]bool{
 	"KQUEUE":  true,
 	"a_inode": true,
 }
 
 // SnapshotOpenFiles will capture the list of currently open-files for the process.
 //
-//nolint:wrapcheck
+//nolint:wrapcheck // FIXME: work in progress
 func SnapshotOpenFiles(file *os.File) ([]byte, error) {
-	// Using a buffer would add a pipe to the list of files
-	// Reimplement this stuff in go ASAP and toss lsof instead of passing around fd
+	// Using a buffer would add a pipe to the list of files.
+	// Reimplement this stuff in go ASAP and toss lsof instead of passing around fd.
 	_, _ = file.Seek(0, 0)
 	_ = file.Truncate(0)
 
@@ -48,7 +48,7 @@ func SnapshotOpenFiles(file *os.File) ([]byte, error) {
 		return nil, err
 	}
 
-	//nolint:gosec
+	//nolint:gosec // G204 is fine here
 	cmd := exec.Command(exe, "-nP", "-p", strconv.Itoa(syscall.Getpid()))
 	cmd.Stdout = file
 

mod/tigron/internal/highk/goroutines.go

@@ -22,7 +22,7 @@ import (
 
 // FindGoRoutines retrieves leaked go routines, which are returned as an error.
 //
-//nolint:wrapcheck
+//nolint:wrapcheck // FIXME: work in progress
 func FindGoRoutines() error {
 	return goleak.Find()
 }