Enable more revive lint rules (#1511)

.golangci.yml

@@ -51,6 +51,22 @@ linters-settings:
         disabled: false
         arguments:
           - "preserveScope"
+      - name: package-comments
+        disabled: false
+      - name: context-as-argument
+        disabled: false
+      - name: context-keys-type
+        disabled: false
+      - name: error-return
+        disabled: false
+      - name: errorf
+        disabled: false
+      - name: unreachable-code
+        disabled: false
+      - name: early-return
+        disabled: false
+      - name: confusing-naming
+        disabled: false
 
 issues:
   exclude-rules:

cmd/launcher/launcher.go

@@ -353,8 +353,8 @@ func runLauncher(ctx context.Context, cancel func(), slogger, systemSlogger *mul
 
 		// create notification consumer
 		notificationConsumer, err := notificationconsumer.NewNotifyConsumer(
-			runner,
 			ctx,
+			runner,
 			notificationconsumer.WithLogger(logger),
 		)
 		if err != nil {

ee/control/consumers/notificationconsumer/notify_consumer.go

@@ -39,7 +39,7 @@ func WithLogger(logger log.Logger) notificationConsumerOption {
 	}
 }
 
-func NewNotifyConsumer(runner *desktopRunner.DesktopUsersProcessesRunner, ctx context.Context, opts ...notificationConsumerOption) (*NotificationConsumer, error) {
+func NewNotifyConsumer(ctx context.Context, runner *desktopRunner.DesktopUsersProcessesRunner, opts ...notificationConsumerOption) (*NotificationConsumer, error) {
 	nc := &NotificationConsumer{
 		runner: runner,
 		logger: log.NewNopLogger(),

ee/desktop/user/notify/notify_linux.go

@@ -94,11 +94,12 @@ func (d *dbusNotifier) Listen() error {
 					level.Warn(d.logger).Log("msg", "couldn't create command to start process", "err", err, "browser_launcher", browserLauncher)
 					continue
 				}
-				if err := cmd.Start(); err != nil {
-					level.Error(d.logger).Log("msg", "couldn't start process", "err", err, "browser_launcher", browserLauncher)
-				} else {
+
+				err = cmd.Start()
+				if err == nil {
 					break
 				}
+				level.Error(d.logger).Log("msg", "couldn't start process", "err", err, "browser_launcher", browserLauncher)
 			}
 
 		case <-d.interrupt:

ee/tables/ioreg/ioreg.go

@@ -7,7 +7,6 @@
 // As the returned data is a complex nested plist, this uses the
 // dataflatten tooling. (See
 // https://godoc.org/github.com/kolide/launcher/ee/dataflatten)
-
 package ioreg
 
 import (

ee/tables/mdmclient/mdmclient.go

@@ -4,7 +4,6 @@
 // Package mdmclient provides a table that parses the mdmclient
 // output. Empirically, this seems to be an almost gnustep
 // plist. With some headers. So, unwind that.
-
 package mdmclient
 
 import (

ee/tables/profiles/profiles.go

@@ -7,7 +7,6 @@
 // As the returned data is a complex nested plist, this uses the
 // dataflatten tooling. (See
 // https://godoc.org/github.com/kolide/launcher/ee/dataflatten)
-
 package profiles
 
 import (

ee/tables/pwpolicy/pwpolicy.go

@@ -7,7 +7,6 @@
 // As the returned data is a complex nested plist, this uses the
 // dataflatten tooling. (See
 // https://godoc.org/github.com/kolide/launcher/ee/dataflatten)
-
 package pwpolicy
 
 import (

ee/tables/systemprofiler/systemprofiler.go

@@ -14,27 +14,26 @@
 //
 // Everything, minimal details:
 //
-//    osquery> select count(*) from kolide_system_profiler where datatype like "%" and detaillevel = "mini";
-//    +----------+
-//    | count(*) |
-//    +----------+
-//    | 1270     |
-//    +----------+
+//	osquery> select count(*) from kolide_system_profiler where datatype like "%" and detaillevel = "mini";
+//	+----------+
+//	| count(*) |
+//	+----------+
+//	| 1270     |
+//	+----------+
 //
 // Multiple data types (slightly redacted):
 //
-//    osquery> select fullkey, key, value, datatype from kolide_system_profiler where datatype in ("SPCameraDataType", "SPiBridgeDataType");
-//    +----------------------+--------------------+------------------------------------------+-------------------+
-//    | fullkey              | key                | value                                    | datatype          |
-//    +----------------------+--------------------+------------------------------------------+-------------------+
-//    | 0/spcamera_unique-id | spcamera_unique-id | 0x1111111111111111                       | SPCameraDataType  |
-//    | 0/_name              | _name              | FaceTime HD Camera                       | SPCameraDataType  |
-//    | 0/spcamera_model-id  | spcamera_model-id  | UVC Camera VendorID_1452 ProductID_30000 | SPCameraDataType  |
-//    | 0/_name              | _name              | Controller Information                   | SPiBridgeDataType |
-//    | 0/ibridge_build      | ibridge_build      | 14Y000                                   | SPiBridgeDataType |
-//    | 0/ibridge_model_name | ibridge_model_name | Apple T1 Security Chip                   | SPiBridgeDataType |
-//    +----------------------+--------------------+------------------------------------------+-------------------+
-
+//	osquery> select fullkey, key, value, datatype from kolide_system_profiler where datatype in ("SPCameraDataType", "SPiBridgeDataType");
+//	+----------------------+--------------------+------------------------------------------+-------------------+
+//	| fullkey              | key                | value                                    | datatype          |
+//	+----------------------+--------------------+------------------------------------------+-------------------+
+//	| 0/spcamera_unique-id | spcamera_unique-id | 0x1111111111111111                       | SPCameraDataType  |
+//	| 0/_name              | _name              | FaceTime HD Camera                       | SPCameraDataType  |
+//	| 0/spcamera_model-id  | spcamera_model-id  | UVC Camera VendorID_1452 ProductID_30000 | SPCameraDataType  |
+//	| 0/_name              | _name              | Controller Information                   | SPiBridgeDataType |
+//	| 0/ibridge_build      | ibridge_build      | 14Y000                                   | SPiBridgeDataType |
+//	| 0/ibridge_model_name | ibridge_model_name | Apple T1 Security Chip                   | SPiBridgeDataType |
+//	+----------------------+--------------------+------------------------------------------+-------------------+
 package systemprofiler
 
 import (

pkg/make/builder.go

@@ -1,6 +1,5 @@
 /*
-	Package make provides some simple functions to handle build and go
-
+Package make provides some simple functions to handle build and go
 dependencies.
 
 We used to do this with gnumake rules, but as we added windows

pkg/make/builder_test.go

@@ -17,14 +17,22 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+type contextKey string
+
+func (c contextKey) String() string {
+	return string(c)
+}
+
+const contextKeyEnv contextKey = "ENV"
+
 func helperCommandContext(ctx context.Context, command string, args ...string) (cmd *exec.Cmd) {
 	cs := []string{"-test.run=TestHelperProcess", "--", command}
 	cs = append(cs, args...)
 	cmd = exec.CommandContext(ctx, os.Args[0], cs...) //nolint:forbidigo // Fine to use exec.CommandContext in tests
 	cmd.Env = []string{"GO_WANT_HELPER_PROCESS=1"}
 
 	// Do we have an ENV key? (type assert)
-	if ctxEnv, ok := ctx.Value("ENV").([]string); ok {
+	if ctxEnv, ok := ctx.Value(contextKeyEnv).([]string); ok {
 		cmd.Env = append(cmd.Env, ctxEnv...)
 	}
 
@@ -191,7 +199,8 @@ func TestGetVersion(t *testing.T) {
 
 			ctx, cancel := context.WithCancel(context.Background())
 			defer cancel()
-			ctx = context.WithValue(ctx, "ENV", []string{fmt.Sprintf("FAKE_GIT_DESCRIBE=%s", tt.in)})
+
+			ctx = context.WithValue(ctx, contextKeyEnv, []string{fmt.Sprintf("FAKE_GIT_DESCRIBE=%s", tt.in)})
 			os.Setenv("FAKE_GIT_DESCRIBE", tt.in)
 			ver, err := b.getVersion(ctx)
 			if tt.err == true {

pkg/osquery/runsimple/osqueryrunner.go

@@ -1,6 +1,5 @@
 // Package runsimple is meant as a simple runner for osquery. It is initial just handling one off executions, but may
 // perhaps, expand to also handling daemonization
-
 package runsimple
 
 import (

pkg/osquery/runtime/history/storage.go

@@ -19,13 +19,12 @@ func (h *History) load() error {
 
 	var instances []*Instance
 
-	if instancesBytes != nil {
-		if err := json.Unmarshal(instancesBytes, &instances); err != nil {
-			return fmt.Errorf("error unmarshalling osquery_instance_history: %w", err)
-		}
-	} else {
+	if instancesBytes == nil {
 		return nil
 	}
+	if err := json.Unmarshal(instancesBytes, &instances); err != nil {
+		return fmt.Errorf("error unmarshalling osquery_instance_history: %w", err)
+	}
 
 	h.instances = instances
 	return nil

pkg/osquery/runtime/runner.go

@@ -546,24 +546,22 @@ func (r *Runner) launchOsqueryInstance() error {
 				// better for a Limiter
 				maxHealthChecks := 5
 				for i := 1; i <= maxHealthChecks; i++ {
-					if err := r.Healthy(); err != nil {
-						if i == maxHealthChecks {
-							level.Info(o.logger).Log("msg", "Health check failed. Giving up", "attempt", i, "err", err)
-							return fmt.Errorf("health check failed: %w", err)
-						}
-
-						level.Debug(o.logger).Log("msg", "Health check failed. Will retry", "attempt", i, "err", err)
-						time.Sleep(1 * time.Second)
-
-					} else {
-						// err was nil, clear failed
+					err := r.Healthy()
+					if err == nil {
+						// err was nil, clear failed attempts
 						if i > 1 {
 							level.Debug(o.logger).Log("msg", "Health check passed. Clearing error", "attempt", i)
 						}
-
 						break
 					}
 
+					if i == maxHealthChecks {
+						level.Info(o.logger).Log("msg", "Health check failed. Giving up", "attempt", i, "err", err)
+						return fmt.Errorf("health check failed: %w", err)
+					}
+
+					level.Debug(o.logger).Log("msg", "Health check failed. Will retry", "attempt", i, "err", err)
+					time.Sleep(1 * time.Second)
 				}
 			}
 		}

pkg/osquery/table/platform_tables.go

@@ -9,8 +9,8 @@ import (
 	"github.com/osquery/osquery-go/plugin/table"
 )
 
-// platformTables returns an empty set. It's here as a catchall for
+// platformSpecificTables returns an empty set. It's here as a catchall for
 // unimplemented platforms.
-func platformTables(client *osquery.ExtensionManagerClient, logger log.Logger, currentOsquerydBinaryPath string) []*table.Plugin {
+func platformSpecificTables(client *osquery.ExtensionManagerClient, logger log.Logger, currentOsquerydBinaryPath string) []*table.Plugin {
 	return []*table.Plugin{}
 }

pkg/osquery/table/platform_tables_darwin.go

@@ -37,7 +37,7 @@ const (
 	screenlockQuery    = "select enabled, grace_period from screenlock"
 )
 
-func platformTables(logger log.Logger, currentOsquerydBinaryPath string) []osquery.OsqueryPlugin {
+func platformSpecificTables(logger log.Logger, currentOsquerydBinaryPath string) []osquery.OsqueryPlugin {
 	munki := munki.New()
 
 	// This table uses undocumented APIs, There is some discussion at the

pkg/osquery/table/platform_tables_linux.go

@@ -28,7 +28,7 @@ import (
 	osquery "github.com/osquery/osquery-go"
 )
 
-func platformTables(logger log.Logger, currentOsquerydBinaryPath string) []osquery.OsqueryPlugin {
+func platformSpecificTables(logger log.Logger, currentOsquerydBinaryPath string) []osquery.OsqueryPlugin {
 	return []osquery.OsqueryPlugin{
 		cryptsetup.TablePlugin(logger),
 		gsettings.Settings(logger),

pkg/osquery/table/platform_tables_windows.go

@@ -17,7 +17,7 @@ import (
 	osquery "github.com/osquery/osquery-go"
 )
 
-func platformTables(logger log.Logger, currentOsquerydBinaryPath string) []osquery.OsqueryPlugin {
+func platformSpecificTables(logger log.Logger, currentOsquerydBinaryPath string) []osquery.OsqueryPlugin {
 	return []osquery.OsqueryPlugin{
 		ProgramIcons(),
 		dsim_default_associations.TablePlugin(logger),

pkg/osquery/table/table.go

@@ -60,7 +60,7 @@ func PlatformTables(logger log.Logger, currentOsquerydBinaryPath string) []osque
 	tables = append(tables, dataflattentable.AllTablePlugins(logger)...)
 
 	// add in the platform specific ones (as denoted by build tags)
-	tables = append(tables, platformTables(logger, currentOsquerydBinaryPath)...)
+	tables = append(tables, platformSpecificTables(logger, currentOsquerydBinaryPath)...)
 
 	return tables
 }

pkg/traces/traces.go

@@ -27,7 +27,7 @@ const (
 // ending the span. `keyVals` should be a list of pairs, where the first in the pair is a
 // string representing the attribute key and the second in the pair is the attribute value.
 func StartHttpRequestSpan(r *http.Request, keyVals ...interface{}) (*http.Request, trace.Span) {
-	ctx, span := startSpan(r.Context(), keyVals...)
+	ctx, span := startSpanWithExtractedAttributes(r.Context(), keyVals...)
 	return r.WithContext(ctx), span
 }
 
@@ -36,12 +36,12 @@ func StartHttpRequestSpan(r *http.Request, keyVals ...interface{}) (*http.Reques
 // ending the span. `keyVals` should be a list of pairs, where the first in the pair is a
 // string representing the attribute key and the second in the pair is the attribute value.
 func StartSpan(ctx context.Context, keyVals ...interface{}) (context.Context, trace.Span) {
-	return startSpan(ctx, keyVals...)
+	return startSpanWithExtractedAttributes(ctx, keyVals...)
 }
 
-// startSpan is the internal implementation of StartSpan and StartHttpRequestSpan with runtime.Caller(2)
-// so that the caller of the wrapper function is used.
-func startSpan(ctx context.Context, keyVals ...interface{}) (context.Context, trace.Span) {
+// startSpanWithExtractedAttributes is the internal implementation of StartSpan and StartHttpRequestSpan
+// with runtime.Caller(2) so that the caller of the wrapper function is used.
+func startSpanWithExtractedAttributes(ctx context.Context, keyVals ...interface{}) (context.Context, trace.Span) {
 	spanName := defaultSpanName
 
 	opts := make([]trace.SpanStartOption, 0)

pkg/windows/oleconv/oleconv.go

@@ -3,7 +3,6 @@
 //
 // It is originally from
 // https://github.com/ceshihao/windowsupdate/blob/master/oleconv.go
-
 package oleconv
 
 import (