Cosmetic cleanup

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)
- some tests enhanced with added info

And:
- adding a WhiteList test for internal/com

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

Author: apostasie

mod/tigron/.golangci.yml

@@ -12,6 +12,28 @@ issues:
 
 linters:
   default: all
+  enable:
+    # These are the default set of golangci
+    - errcheck          # Errcheck is a program for checking for unchecked errors in Go code. These unchecked errors can be critical bugs in some cases.
+    - 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
+    - forcetypeassert   # not asserting is risky and bad influence for newcomers
+    - 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
@@ -31,6 +53,31 @@ linters:
     - 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
     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,
 	}