Add some precommit checks which cover things integration is doing (#138)

* Cleanup a bunch of lints found. Mostly not checking return values. * Add vscode settings for the things we require (coverage and the lint tool). Easier for adopters to avoid local setup
Snowflake-Labs · Jun 17, 2022 · 283fed7 · 283fed7
1 parent aeb4cdd
commit 283fed7
Show file tree

Hide file tree

Showing 18 changed files with 193 additions and 99 deletions.
diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml
@@ -3,14 +3,27 @@ name: PR
 on:
   - pull_request
 jobs:
+  pre-commit:
+    name: Pre commit
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v3
+    - uses: actions/setup-go@331ce1d993939866bb63c32c6cbbfd48fa76fc57
+      with:
+        go-version: '^1.18'
+    - name: Install tools
+      run: |
+        curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.46.2
+    - uses: actions/setup-python@v3
+    - uses: pre-commit/[email protected]
   test:
     name: Unit tests
     runs-on: ubuntu-20.04
     steps:
       - uses: actions/checkout@5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f
       - uses: actions/setup-go@331ce1d993939866bb63c32c6cbbfd48fa76fc57
         with:
-          go-version: '^1.17'
+          go-version: '^1.18'
       - name: Install tools
         run: |
           sudo apt-get update

diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
@@ -0,0 +1,9 @@
+repos:
+- repo: https://github.com/dnephin/pre-commit-golang
+  rev: v0.5.0
+  hooks:
+    - id: go-fmt
+    - id: golangci-lint
+      args: [--timeout=5m]
+    - id: go-build
+    - id: go-mod-tidy
diff --git a/.vscode/settings.json b/.vscode/settings.json
@@ -0,0 +1,7 @@
+{
+    "go.coverOnSave": true,
+    "go.coverOnSingleTest": true,
+    "go.coverOnSingleTestFile": true,
+    "go.lintOnSave": "workspace",
+    "go.lintTool": "golangci-lint",
+}
diff --git a/auth/mtls/mtls.go b/auth/mtls/mtls.go
@@ -82,7 +82,7 @@ func (w *WrappedTransportCredentials) checkRefresh() error {
 		}
 		w.creds = newCreds
 		if w.serverName != "" {
-			return w.creds.OverrideServerName(w.serverName)
+			return w.creds.OverrideServerName(w.serverName) //nolint:staticcheck
 		}
 	}
 	return nil
@@ -106,13 +106,15 @@ func (w *WrappedTransportCredentials) ServerHandshake(n net.Conn) (net.Conn, cre
 
 // Info -- see credentials.Info
 func (w *WrappedTransportCredentials) Info() credentials.ProtocolInfo {
-	w.checkRefresh()
+	// We have no way to process an error with this API
+	_ = w.checkRefresh()
 	return w.creds.Info()
 }
 
 // Clone -- see credentials.Clone
 func (w *WrappedTransportCredentials) Clone() credentials.TransportCredentials {
-	w.checkRefresh()
+	// We have no way to process an error with this API
+	_ = w.checkRefresh()
 	wrapped := &WrappedTransportCredentials{
 		creds:      w.creds.Clone(),
 		loaderName: w.loaderName,
@@ -128,7 +130,7 @@ func (w *WrappedTransportCredentials) OverrideServerName(s string) error {
 		return err
 	}
 	w.serverName = s
-	return w.creds.OverrideServerName(s)
+	return w.creds.OverrideServerName(s) //nolint:staticcheck
 }
 
 // Register associates a name with a mechanism for loading credentials.

diff --git a/auth/mtls/mtls_test.go b/auth/mtls/mtls_test.go
@@ -133,7 +133,8 @@ func (s *simpleLoader) CertsRefreshed() bool {
 
 func serverWithPolicy(t *testing.T, policy string) (*bufconn.Listener, *grpc.Server) {
 	t.Helper()
-	Register("refresh", &simpleLoader{name: "refresh"})
+	err := Register("refresh", &simpleLoader{name: "refresh"})
+	testutil.FatalOnErr("Register", err, t)
 	creds, err := LoadServerCredentials(context.Background(), "refresh")
 	testutil.FatalOnErr("Failed to load server cert", err, t)
 	lis := bufconn.Listen(bufSize)
@@ -193,10 +194,14 @@ func TestLoadRootOfTrust(t *testing.T) {
 
 func TestLoadClientServerCredentials(t *testing.T) {
 	unregisterAll()
-	Register("simple", &simpleLoader{name: "simple"})
-	Register("errorCA", &simpleLoader{name: "errorCA"})
-	Register("errorCert", &simpleLoader{name: "errorCert"})
-	Register("refresh", &simpleLoader{name: "refresh"})
+	err := Register("simple", &simpleLoader{name: "simple"})
+	testutil.FatalOnErr("Register", err, t)
+	err = Register("errorCA", &simpleLoader{name: "errorCA"})
+	testutil.FatalOnErr("Register", err, t)
+	err = Register("errorCert", &simpleLoader{name: "errorCert"})
+	testutil.FatalOnErr("Register", err, t)
+	err = Register("refresh", &simpleLoader{name: "refresh"})
+	testutil.FatalOnErr("Register", err, t)
 
 	for _, tc := range []struct {
 		name    string
@@ -232,12 +237,14 @@ func TestLoadClientServerCredentials(t *testing.T) {
 			server, err := LoadServerCredentials(context.Background(), tc.loader)
 			testutil.WantErr("server", err, tc.wantErr, t)
 			if !tc.wantErr {
-				server.OverrideServerName("server")
+				err = server.OverrideServerName("server") //nolint:staticcheck
+				testutil.FatalOnErr("OverrideServerName", err, t)
 			}
 			client, err := LoadClientCredentials(context.Background(), tc.loader)
 			testutil.WantErr("client", err, tc.wantErr, t)
 			if !tc.wantErr {
-				client.OverrideServerName("server")
+				err = client.OverrideServerName("server") //nolint:staticcheck
+				testutil.FatalOnErr("OverrideServerName", err, t)
 			}
 		})
 	}
@@ -247,10 +254,12 @@ func TestHealthCheck(t *testing.T) {
 	var err error
 	ctx := context.Background()
 	unregisterAll()
-	Register("refresh", &simpleLoader{name: "refresh"})
+	err = Register("refresh", &simpleLoader{name: "refresh"})
+	testutil.FatalOnErr("Register", err, t)
 	creds, err := LoadClientCredentials(ctx, "refresh")
-	creds.OverrideServerName("bufnet")
 	testutil.FatalOnErr("Failed to load client cert", err, t)
+	err = creds.OverrideServerName("bufnet") //nolint:staticcheck
+	testutil.FatalOnErr("OverrideServerName", err, t)
 	for _, tc := range []struct {
 		name   string
 		policy string
@@ -279,6 +288,7 @@ func TestHealthCheck(t *testing.T) {
 	} {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
+			unregisterAll()
 			l, s := serverWithPolicy(t, tc.policy)
 			conn, err := grpc.DialContext(ctx, "bufnet", grpc.WithContextDialer(bufDialer(l)), grpc.WithTransportCredentials(creds))
 			testutil.FatalOnErr("Failed to dial bufnet", err, t)

diff --git a/client/client.go b/client/client.go
@@ -45,7 +45,9 @@ func SetupSubpackage(name string, f *flag.FlagSet) *subcommands.Commander {
 func GenerateSynopsis(c *subcommands.Commander, leading int) string {
 	b := &bytes.Buffer{}
 	w := tabwriter.NewWriter(b, 8, 0, 2, ' ', 0)
-	w.Write([]byte("\n"))
+	if _, err := w.Write([]byte("\n")); err != nil {
+		panic(fmt.Sprintf("buffer write failed: %v", err))
+	}
 	fn := func(c *subcommands.CommandGroup, comm subcommands.Command) {
 		switch comm.Name() {
 		case "help", "flags", "commands":

diff --git a/proxy/proxy/proxy_test.go b/proxy/proxy/proxy_test.go
@@ -52,7 +52,7 @@ func startTestProxy(ctx context.Context, t *testing.T, targets map[string]*bufco
 	go func() {
 		// Don't care about errors here as they might come on shutdown and we
 		// can't log through t at that point anyways.
-		grpcServer.Serve(lis)
+		_ = grpcServer.Serve(lis)
 	}()
 	t.Cleanup(func() {
 		grpcServer.Stop()
@@ -388,7 +388,7 @@ func TestWithFakeServerForErrors(t *testing.T) {
 	go func() {
 		// Don't care about errors here as they might come on shutdown and we
 		// can't log through t at that point anyways.
-		grpcServer.Serve(lis)
+		_ = grpcServer.Serve(lis)
 	}()
 	t.Cleanup(func() {
 		grpcServer.Stop()
@@ -404,7 +404,7 @@ func TestWithFakeServerForErrors(t *testing.T) {
 		if err != nil {
 			return err
 		}
-		stream.Send(&proxypb.ProxyReply{
+		err = stream.Send(&proxypb.ProxyReply{
 			Reply: &proxypb.ProxyReply_StartStreamReply{
 				StartStreamReply: &proxypb.StartStreamReply{
 					Target: req.GetStartStream().Target,
@@ -415,14 +415,15 @@ func TestWithFakeServerForErrors(t *testing.T) {
 				},
 			},
 		})
+		tu.FatalOnErr("stream.Send", err, t)
 		return nil
 	}
 	replyWithBadNonce := func(stream proxypb.Proxy_ProxyServer) error {
 		req, err := stream.Recv()
 		if err != nil {
 			return err
 		}
-		stream.Send(&proxypb.ProxyReply{
+		err = stream.Send(&proxypb.ProxyReply{
 			Reply: &proxypb.ProxyReply_StartStreamReply{
 				StartStreamReply: &proxypb.StartStreamReply{
 					Target: req.GetStartStream().Target,
@@ -433,19 +434,22 @@ func TestWithFakeServerForErrors(t *testing.T) {
 				},
 			},
 		})
+		tu.FatalOnErr("stream.Send", err, t)
 		return nil
 	}
 	setupThenError := func(stream proxypb.Proxy_ProxyServer) error {
-		channelSetup(stream)
-		_, err := stream.Recv()
+		err := channelSetup(stream)
+		tu.FatalOnErr("channelSetup", err, t)
+		_, err = stream.Recv()
 		if err != nil {
 			return err
 		}
 		return errors.New("error")
 	}
 	setupThenEOF := func(stream proxypb.Proxy_ProxyServer) error {
-		channelSetup(stream)
-		_, err := stream.Recv()
+		err := channelSetup(stream)
+		tu.FatalOnErr("channelSetup", err, t)
+		_, err = stream.Recv()
 		if err != nil {
 			return err
 		}
@@ -457,72 +461,80 @@ func TestWithFakeServerForErrors(t *testing.T) {
 		if err != nil {
 			return err
 		}
-		stream.Send(&proxypb.ProxyReply{
+		err = stream.Send(&proxypb.ProxyReply{
 			Reply: &proxypb.ProxyReply_ServerClose{},
 		})
+		tu.FatalOnErr("stream.Send", err, t)
 		return nil
 	}
 	nonMatchingData := func(stream proxypb.Proxy_ProxyServer) error {
 		_, err := stream.Recv()
 		if err != nil {
 			return err
 		}
-		stream.Send(&proxypb.ProxyReply{
+		err = stream.Send(&proxypb.ProxyReply{
 			Reply: &proxypb.ProxyReply_StartStreamReply{
 				StartStreamReply: &proxypb.StartStreamReply{},
 			},
 		})
+		tu.FatalOnErr("stream.Send", err, t)
 		return nil
 	}
 	dataPacketWrongID := func(stream proxypb.Proxy_ProxyServer) error {
-		channelSetup(stream)
-		_, err := stream.Recv()
+		err := channelSetup(stream)
+		tu.FatalOnErr("channelSetup", err, t)
+		_, err = stream.Recv()
 		if err != nil {
 			return err
 		}
-		stream.Send(&proxypb.ProxyReply{
+		err = stream.Send(&proxypb.ProxyReply{
 			Reply: &proxypb.ProxyReply_StreamData{
 				StreamData: &proxypb.StreamData{
 					StreamIds: []uint64{1},
 					Payload:   &anypb.Any{},
 				},
 			},
 		})
+		tu.FatalOnErr("stream.Send", err, t)
+
 		return nil
 	}
 	closePacketWrongID := func(stream proxypb.Proxy_ProxyServer) error {
-		channelSetup(stream)
-		_, err := stream.Recv()
+		err := channelSetup(stream)
+		tu.FatalOnErr("channelSetup", err, t)
+		_, err = stream.Recv()
 		if err != nil {
 			return err
 		}
-		stream.Send(&proxypb.ProxyReply{
+		err = stream.Send(&proxypb.ProxyReply{
 			Reply: &proxypb.ProxyReply_ServerClose{
 				ServerClose: &proxypb.ServerClose{
 					StreamIds: []uint64{1},
 				},
 			},
 		})
+		tu.FatalOnErr("stream.Send", err, t)
 		return nil
 	}
 	closePacketNil := func(stream proxypb.Proxy_ProxyServer) error {
 		_, err := stream.Recv()
 		if err != nil {
 			return err
 		}
-		stream.Send(&proxypb.ProxyReply{
+		err = stream.Send(&proxypb.ProxyReply{
 			Reply: &proxypb.ProxyReply_ServerClose{
 				ServerClose: nil,
 			},
 		})
+		tu.FatalOnErr("stream.Send", err, t)
 		return nil
 	}
 	closeInvalidStream := func(stream proxypb.Proxy_ProxyServer) error {
 		_, err := stream.Recv()
 		if err != nil {
 			return err
 		}
-		stream.Send(&proxypb.ProxyReply{
+		err = stream.Send(&proxypb.ProxyReply{
 			Reply: &proxypb.ProxyReply_ServerClose{
 				ServerClose: &proxypb.ServerClose{
 					StreamIds: []uint64{1},
@@ -533,46 +545,52 @@ func TestWithFakeServerForErrors(t *testing.T) {
 				},
 			},
 		})
+		tu.FatalOnErr("stream.Send", err, t)
 		return nil
 	}
 	badReply := func(stream proxypb.Proxy_ProxyServer) error {
 		_, err := stream.Recv()
 		if err != nil {
 			return err
 		}
-		stream.Send(&proxypb.ProxyReply{
+		err = stream.Send(&proxypb.ProxyReply{
 			Reply: &proxypb.ProxyReply_StreamData{},
 		})
+		tu.FatalOnErr("stream.Send", err, t)
 		return nil
 	}
 	badPacket := func(stream proxypb.Proxy_ProxyServer) error {
-		channelSetup(stream)
-		_, err := stream.Recv()
+		err := channelSetup(stream)
+		tu.FatalOnErr("channelSetup", err, t)
+		_, err = stream.Recv()
 		if err != nil {
 			return err
 		}
-		stream.Send(&proxypb.ProxyReply{})
+		err = stream.Send(&proxypb.ProxyReply{})
+		tu.FatalOnErr("stream.Send", err, t)
 		return nil
 	}
 	validReplyThenCloseError := func(stream proxypb.Proxy_ProxyServer) error {
-		channelSetup(stream)
-		_, err := stream.Recv()
+		err := channelSetup(stream)
+		tu.FatalOnErr("channelSetup", err, t)
+		_, err = stream.Recv()
 		if err != nil {
 			return err
 		}
 		payload, err := anypb.New(&emptypb.Empty{})
 		if err != nil {
 			return err
 		}
-		stream.Send(&proxypb.ProxyReply{
+		err = stream.Send(&proxypb.ProxyReply{
 			Reply: &proxypb.ProxyReply_StreamData{
 				StreamData: &proxypb.StreamData{
 					Payload:   payload,
 					StreamIds: []uint64{0},
 				},
 			},
 		})
-		stream.Send(&proxypb.ProxyReply{
+		tu.FatalOnErr("stream.Send", err, t)
+		err = stream.Send(&proxypb.ProxyReply{
 			Reply: &proxypb.ProxyReply_ServerClose{
 				ServerClose: &proxypb.ServerClose{
 					Status: &proxypb.Status{
@@ -582,6 +600,7 @@ func TestWithFakeServerForErrors(t *testing.T) {
 				},
 			},
 		})
+		tu.FatalOnErr("stream.Send", err, t)
 		_, err = stream.Recv()
 		if err != nil {
 			return err