House Cleaning Part 2 - Electric Boogaloo! (#133)

* Get rid of nolint:wsl comments

We do not use wsl so no need to ignore its errors.

* Get rid of nolint:gomnd comments

We do not use gomnd so no need to ignore its errors.

* Get rid of nolint:gocyclo comments that are unnecessary

* Get rid of nolint:gocritic comments

2 ignores needed some small code changes to make it possible to avoid
commenting gocritic out. Both in tests and both solved by using stdlib
helpers.

I replaced a os.MkdirTemp() with t.TempDir() that gets cleaned up when
the test is done, this avoids needing to clean up the sub-dirs. I could
have cleaned up tmpDir instead of using t.TempDir for the same effect
but its better to let the stdlib do it if it wants to.

The other change was to run loop tests under t.Run() so that gocritic
stops complaining about defer-in-a-loop cases. This was probably the
better option from the beginning anyway since it lets us test
individually failing cases easily.

* Get rid of pure nolinit comments

Some lints may be relevant even in test fixture code.

* Get rid of nolint:errcheck comments

This one had me second guessing myself quite a lot. Turns out defer
can't overwrite the return value so these are all not doing what it
looks like they're doing[1]. In order to influence the return'd error we
would have to used named return values so that err could be changed.

I was going to just do a simple defer'd recover like:

```
func() { _ = recover() }()
```

to keep catching the panics and get the same result as today but decided
to just do what was intendend instead.

1: https://go.dev/play/p/UIVjXdqajxI

* Fix CODEOWNERS and move to .github directory

The file was previously incorrect, they were specifying teams not
people. Even if the names were correct the file would still not match
intention since it is interpreted line-by-line which means only
@joelrebel would have been used as the CODEOWNER.

This commit fixes those errors, add myself and Nacho as owners and moves
the GitHub centric file to the .github dir where other similar files
already exist.

* fillzero: Drop min

We are now on go 1.22 so can use min from stdlib.

.github/CODEOWNERS

@@ -0,0 +1 @@
+* @joelrebel @mmlb @splaspood @turegano-equinix 

actions/inventory.go

@@ -66,8 +66,6 @@ type Collectors struct {
 }
 
 // Empty returns a bool value
-//
-//nolint:gocyclo  // it's fine
 func (c *Collectors) Empty() bool {
 	if c.InventoryCollector == nil &&
 		c.NICCollector == nil &&
@@ -343,14 +341,11 @@ func (a *InventoryCollectorAction) setDefaultAttributes() {
 
 // CollectDrives executes drive collectors and merges the data into device.[]*Drive
 // nolint:gocyclo // TODO(joel) if theres more conditionals to be added in here, the method is to be split up.
-func (a *InventoryCollectorAction) CollectDrives(ctx context.Context) error {
-	// nolint:errcheck // deferred method catches a panic, error check not required.
-	defer func() error {
+func (a *InventoryCollectorAction) CollectDrives(ctx context.Context) (err error) {
+	defer func() {
 		if r := recover(); r != nil && a.failOnError {
-			return errors.Wrap(ErrPanic, string(debug.Stack()))
+			err = errors.Wrap(ErrPanic, string(debug.Stack()))
 		}
-
-		return nil
 	}()
 
 	if a.collectors.DriveCollectors == nil {
@@ -441,14 +436,11 @@ func (a *InventoryCollectorAction) findDriveByLogicalName(logicalName string, dr
 // CollectDriveCapabilities executes drive capability collectors
 //
 // The capability collector is identified based on the drive logical name.
-func (a *InventoryCollectorAction) CollectDriveCapabilities(ctx context.Context) error {
-	// nolint:errcheck // deferred method catches a panic, error check not required.
-	defer func() error {
+func (a *InventoryCollectorAction) CollectDriveCapabilities(ctx context.Context) (err error) {
+	defer func() {
 		if r := recover(); r != nil && a.failOnError {
-			return errors.Wrap(ErrPanic, string(debug.Stack()))
+			err = errors.Wrap(ErrPanic, string(debug.Stack()))
 		}
-
-		return nil
 	}()
 
 	for _, drive := range a.device.Drives {
@@ -485,16 +477,11 @@ func (a *InventoryCollectorAction) CollectDriveCapabilities(ctx context.Context)
 }
 
 // CollectNICs executes nic collectors and merges the nic data into device.[]*NIC
-//
-// nolint:gocyclo // this is fine for now.
-func (a *InventoryCollectorAction) CollectNICs(ctx context.Context) error {
-	// nolint:errcheck // deferred method catches a panic, error check not required.
-	defer func() error {
+func (a *InventoryCollectorAction) CollectNICs(ctx context.Context) (err error) {
+	defer func() {
 		if r := recover(); r != nil && a.failOnError {
-			return errors.Wrap(ErrPanic, string(debug.Stack()))
+			err = errors.Wrap(ErrPanic, string(debug.Stack()))
 		}
-
-		return nil
 	}()
 
 	if a.collectors.NICCollector == nil {
@@ -536,14 +523,11 @@ func (a *InventoryCollectorAction) CollectNICs(ctx context.Context) error {
 }
 
 // CollectBMC executes the bmc collector and updates device bmc information
-func (a *InventoryCollectorAction) CollectBMC(ctx context.Context) error {
-	// nolint:errcheck // deferred method catches a panic, error check not required.
-	defer func() error {
+func (a *InventoryCollectorAction) CollectBMC(ctx context.Context) (err error) {
+	defer func() {
 		if r := recover(); r != nil && a.failOnError {
-			return errors.Wrap(ErrPanic, string(debug.Stack()))
+			err = errors.Wrap(ErrPanic, string(debug.Stack()))
 		}
-
-		return nil
 	}()
 
 	if a.collectors.BMCCollector == nil {
@@ -573,14 +557,11 @@ func (a *InventoryCollectorAction) CollectBMC(ctx context.Context) error {
 }
 
 // CollectCPLDs executes the bmc collector and updates device cpld information
-func (a *InventoryCollectorAction) CollectCPLDs(ctx context.Context) error {
-	// nolint:errcheck // deferred method catches a panic, error check not required.
-	defer func() error {
+func (a *InventoryCollectorAction) CollectCPLDs(ctx context.Context) (err error) {
+	defer func() {
 		if r := recover(); r != nil && a.failOnError {
-			return errors.Wrap(ErrPanic, string(debug.Stack()))
+			err = errors.Wrap(ErrPanic, string(debug.Stack()))
 		}
-
-		return nil
 	}()
 
 	if a.collectors.CPLDCollector == nil {
@@ -619,14 +600,11 @@ func (a *InventoryCollectorAction) CollectCPLDs(ctx context.Context) error {
 }
 
 // CollectBIOS executes the bios collector and updates device bios information
-func (a *InventoryCollectorAction) CollectBIOS(ctx context.Context) error {
-	// nolint:errcheck // deferred method catches a panic, error check not required.
-	defer func() error {
+func (a *InventoryCollectorAction) CollectBIOS(ctx context.Context) (err error) {
+	defer func() {
 		if r := recover(); r != nil && a.failOnError {
-			return errors.Wrap(ErrPanic, string(debug.Stack()))
+			err = errors.Wrap(ErrPanic, string(debug.Stack()))
 		}
-
-		return nil
 	}()
 
 	if a.collectors.BIOSCollector == nil {
@@ -660,14 +638,11 @@ func (a *InventoryCollectorAction) CollectBIOS(ctx context.Context) error {
 }
 
 // CollectTPMs executes the TPM collector and updates device TPM information
-func (a *InventoryCollectorAction) CollectTPMs(ctx context.Context) error {
-	// nolint:errcheck // deferred method catches a panic, error check not required.
-	defer func() error {
+func (a *InventoryCollectorAction) CollectTPMs(ctx context.Context) (err error) {
+	defer func() {
 		if r := recover(); r != nil && a.failOnError {
-			return errors.Wrap(ErrPanic, string(debug.Stack()))
+			err = errors.Wrap(ErrPanic, string(debug.Stack()))
 		}
-
-		return nil
 	}()
 
 	if a.collectors.TPMCollector == nil {
@@ -705,14 +680,11 @@ func (a *InventoryCollectorAction) CollectTPMs(ctx context.Context) error {
 }
 
 // CollectFirmwareChecksums executes the Firmware checksum collector and updates the component metadata.
-func (a *InventoryCollectorAction) CollectFirmwareChecksums(ctx context.Context) error {
-	// nolint:errcheck // deferred method catches a panic, error check not required.
-	defer func() error {
+func (a *InventoryCollectorAction) CollectFirmwareChecksums(ctx context.Context) (err error) {
+	defer func() {
 		if r := recover(); r != nil && a.failOnError {
-			return errors.Wrap(ErrPanic, string(debug.Stack()))
+			err = errors.Wrap(ErrPanic, string(debug.Stack()))
 		}
-
-		return nil
 	}()
 
 	if a.collectors.FirmwareChecksumCollector == nil {
@@ -751,14 +723,11 @@ func (a *InventoryCollectorAction) CollectFirmwareChecksums(ctx context.Context)
 }
 
 // CollectUEFIVariables executes the UEFI variable collector and stores them on the device object
-func (a *InventoryCollectorAction) CollectUEFIVariables(ctx context.Context) error {
-	// nolint:errcheck // deferred method catches a panic, error check not required.
-	defer func() error {
+func (a *InventoryCollectorAction) CollectUEFIVariables(ctx context.Context) (err error) {
+	defer func() {
 		if r := recover(); r != nil && a.failOnError {
-			return errors.Wrap(ErrPanic, string(debug.Stack()))
+			err = errors.Wrap(ErrPanic, string(debug.Stack()))
 		}
-
-		return nil
 	}()
 
 	if a.collectors.UEFIVarsCollector == nil {
@@ -796,16 +765,11 @@ func (a *InventoryCollectorAction) CollectUEFIVariables(ctx context.Context) err
 }
 
 // CollectStorageControllers executes the StorageControllers collectors and updates device storage controller data
-//
-// nolint:gocyclo // this is fine for now
-func (a *InventoryCollectorAction) CollectStorageControllers(ctx context.Context) error {
-	// nolint:errcheck // deferred method catches a panic, error check not required.
-	defer func() error {
+func (a *InventoryCollectorAction) CollectStorageControllers(ctx context.Context) (err error) {
+	defer func() {
 		if r := recover(); r != nil && a.failOnError {
-			return errors.Wrap(ErrPanic, string(debug.Stack()))
+			err = errors.Wrap(ErrPanic, string(debug.Stack()))
 		}
-
-		return nil
 	}()
 
 	if len(a.collectors.StorageControllerCollectors) == 0 {

firmware/bios_checksum.go

@@ -1,4 +1,3 @@
-//nolint:wsl // it's useless
 package firmware
 
 import (

firmware/bios_checksum_test.go

@@ -1,4 +1,3 @@
-//nolint:all
 package firmware
 
 import (

fixtures/asrr/e3c246d4i-nl.go

@@ -4,8 +4,9 @@ import (
 	"github.com/bmc-toolbox/common"
 )
 
-// nolint //testcode
-// inventory taken with lshw
+// E3C246D4INL is an example inventory taken with lshw
+//
+//nolint:misspell
 var E3C246D4INL = &common.Device{
 	Common: common.Common{
 		Oem:         false,

fixtures/dell/r6515.go

@@ -5,7 +5,7 @@ import (
 	"github.com/metal-toolbox/ironlib/model"
 )
 
-// nolint // test data
+// nolint:dupl,misspell,revive,stylecheck
 var (
 	R6515_oem_components = &model.OemComponents{
 		Dell: []*model.Component{

fixtures/supermicro/x11dph-t.go

@@ -5,7 +5,7 @@ import (
 	"github.com/metal-toolbox/ironlib/model"
 )
 
-// nolint // test data
+// nolint:dupl,misspell,revive,stylecheck
 var (
 	Testdata_X11DPH_T_Components = []*model.Component{
 		{

providers/dell/helpers.go

@@ -58,7 +58,6 @@ func (d *dell) installUpdate(ctx context.Context, updateFile string, downgrade b
 	}
 
 	// set files executable
-	// nolint gocritic: this fs mode declaration is as clear as it gets
 	err := os.Chmod(updateFile, 0o744)
 	if err != nil {
 		return 0, err

utils/dsu_test.go

@@ -86,10 +86,7 @@ func Test_dsuParsePreviewBytes(t *testing.T) {
 func Test_findDSUInventoryCollector(t *testing.T) {
 	invb := "invcol_5N2WM_LN64_20_09_200_921_A00.BIN"
 
-	tmpDir, err := os.MkdirTemp("/tmp", "ironlibtest")
-	if err != nil {
-		t.Error(err)
-	}
+	tmpDir := t.TempDir()
 
 	dirs := []string{
 		tmpDir + "/foo/dsu",
@@ -98,10 +95,7 @@ func Test_findDSUInventoryCollector(t *testing.T) {
 
 	expected := []string{}
 
-	// nolint:gocritic // test code
 	for _, d := range dirs {
-		defer func() { _ = os.RemoveAll(d) }()
-
 		err := os.MkdirAll(d, 0o744)
 		if err != nil {
 			t.Error(err)

utils/executor_test.go

@@ -93,26 +93,27 @@ func initCheckBinTests() []checkBinTester {
 func Test_CheckExecutable(t *testing.T) {
 	tests := initCheckBinTests()
 	for _, c := range tests {
-		if c.createFile {
-			f, err := os.Create(c.filePath)
-			if err != nil {
-				t.Error(err)
-			}
-
-			// nolint:gocritic // test code
-			defer os.Remove(c.filePath)
-
-			if c.fileMode != 0 {
-				err = f.Chmod(fs.FileMode(c.fileMode))
+		t.Run(c.testName, func(t *testing.T) {
+			if c.createFile {
+				f, err := os.Create(c.filePath)
 				if err != nil {
 					t.Error(err)
 				}
+
+				defer os.Remove(c.filePath)
+
+				if c.fileMode != 0 {
+					err = f.Chmod(fs.FileMode(c.fileMode))
+					if err != nil {
+						t.Error(err)
+					}
+				}
 			}
-		}
 
-		e := new(Execute)
-		e.Cmd = c.filePath
-		err := e.CheckExecutable()
-		assert.Equal(t, c.expectedErr, errors.Cause(err), c.testName)
+			e := new(Execute)
+			e.Cmd = c.filePath
+			err := e.CheckExecutable()
+			assert.Equal(t, c.expectedErr, errors.Cause(err), c.testName)
+		})
 	}
 }

utils/fill_zero.go

@@ -84,14 +84,6 @@ func printProgress(totalBytesWritten, partitionSize int64, start *time.Time, byt
 	log.Printf("%s | Progress: %.2f%% | Speed: %.2f MB/s | Estimated time left: %.2f hour(s)\n", path, progress, mbPerSecond, remainingHours)
 }
 
-// We are in go 1.19 min not available yet
-func min(a, b int64) int64 {
-	if a < b {
-		return a
-	}
-	return b
-}
-
 // SetQuiet lowers the verbosity
 func (z *FillZero) SetQuiet() {
 	z.Quiet = true

utils/misc.go

@@ -33,8 +33,7 @@ func copyFile(src, dst string) error {
 		return err
 	}
 
-	// nolint:gomnd // fs mode permissions are easier to read in this form
-	return os.Chmod(dst, os.FileMode(0o600))
+	return os.Chmod(dst, 0o600)
 }
 
 // IdentifyVendorModel returns the device vendor, model, serial number attributes

utils/mlxup.go

@@ -222,7 +222,6 @@ func (m *Mlxup) parseMlxQueryOutput(b []byte) []*MlxupDevice {
 }
 
 // parseMlxDeviceAttributes a mlx device information section into *MlxupDevice
-// nolint:gocyclo // line parsing is cyclomatic
 func parseMlxDeviceAttributes(bSlice [][]byte) *MlxupDevice {
 	device := &MlxupDevice{}
 
@@ -276,8 +275,6 @@ func parseMlxDeviceAttributes(bSlice [][]byte) *MlxupDevice {
 //
 // version["FW"][0] indicates fw version installed
 // version["FW"][1] indicates fw version available
-//
-// nolint:gocyclo // line based parsing is cyclomatic
 func parseMlxVersions(bSlice [][]byte) map[string][]string {
 	versions := map[string][]string{
 		"FW":   make([]string, 0),

utils/smartctl.go

@@ -222,7 +222,6 @@ func smartCtlExitStatus(exitCode int) []string {
 // maskExitCode identifies error bits set based on the smartctl exit code
 // it returns a slice of bits that are set to a value > 0
 // see man 8 smartctl for details
-// nolint:gomnd // comments clarify magic numbers
 func maskExitCode(e int) []int {
 	set := []int{}
 
@@ -280,7 +279,6 @@ func NewFakeSmartctl(dataDir string) *Smartctl {
 	return &Smartctl{Executor: executor}
 }
 
-// nolint:gocyclo // test code
 // ExecWithContext implements the utils.Executor interface
 func (e *FakeSmartctlExecute) ExecWithContext(context.Context) (*Result, error) {
 	switch e.Args[0] {

utils/uefi_firmware_parser.go

@@ -1,4 +1,3 @@
-// nolint: wsl,gocritic
 package utils
 
 import (

utils/uefi_vars.go

@@ -1,4 +1,3 @@
-//nolint:wsl // god it's useless
 package utils
 
 import (