From 283fed799c0ca5be76eba77d57fe194f07525e7a Mon Sep 17 00:00:00 2001 From: James Chacon Date: Fri, 17 Jun 2022 11:14:12 -0700 Subject: [PATCH] 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 --- .github/workflows/pr.yaml | 15 +++- .pre-commit-config.yaml | 9 +++ .vscode/settings.json | 7 ++ auth/mtls/mtls.go | 10 +-- auth/mtls/mtls_test.go | 28 +++++--- client/client.go | 4 +- proxy/proxy/proxy_test.go | 71 ++++++++++++------- proxy/server/server_test.go | 2 +- proxy/server/target_test.go | 3 +- proxy/testutil/testutil.go | 8 ++- services/fdb/client/client.go | 10 ++- services/fdb/server/conf_test.go | 6 +- services/fdb/server/fdbcli_test.go | 25 +++++-- services/localfile/client/client.go | 47 +++++++----- .../localfile/server/localfile_darwin_test.go | 3 +- .../localfile/server/localfile_linux_test.go | 3 +- services/localfile/server/localfile_test.go | 26 ++++--- testing/integrate.sh | 15 +--- 18 files changed, 193 insertions(+), 99 deletions(-) create mode 100644 .pre-commit-config.yaml create mode 100644 .vscode/settings.json diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml index e330b6f6..5e2d0601 100644 --- a/.github/workflows/pr.yaml +++ b/.github/workflows/pr.yaml @@ -3,6 +3,19 @@ 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/action@v3.0.0 test: name: Unit tests runs-on: ubuntu-20.04 @@ -10,7 +23,7 @@ jobs: - 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 new file mode 100644 index 00000000..eab76dd6 --- /dev/null +++ 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 new file mode 100644 index 00000000..7c66b5e4 --- /dev/null +++ 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 index d5bf6d8a..8602824b 100644 --- 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 index 13e7dc7f..e35eec6f 100644 --- 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 index 0e023c24..26d82a09 100644 --- 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 index 8804d2b4..e7cbf38e 100644 --- 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,6 +415,7 @@ func TestWithFakeServerForErrors(t *testing.T) { }, }, }) + tu.FatalOnErr("stream.Send", err, t) return nil } replyWithBadNonce := func(stream proxypb.Proxy_ProxyServer) error { @@ -422,7 +423,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, @@ -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,9 +461,10 @@ 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 { @@ -467,20 +472,22 @@ 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{}, }, }) + 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}, @@ -488,21 +495,25 @@ func TestWithFakeServerForErrors(t *testing.T) { }, }, }) + 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 { @@ -510,11 +521,12 @@ func TestWithFakeServerForErrors(t *testing.T) { 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 { @@ -522,7 +534,7 @@ func TestWithFakeServerForErrors(t *testing.T) { 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,6 +545,7 @@ func TestWithFakeServerForErrors(t *testing.T) { }, }, }) + tu.FatalOnErr("stream.Send", err, t) return nil } badReply := func(stream proxypb.Proxy_ProxyServer) error { @@ -540,23 +553,27 @@ func TestWithFakeServerForErrors(t *testing.T) { 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 } @@ -564,7 +581,7 @@ func TestWithFakeServerForErrors(t *testing.T) { if err != nil { return err } - stream.Send(&proxypb.ProxyReply{ + err = stream.Send(&proxypb.ProxyReply{ Reply: &proxypb.ProxyReply_StreamData{ StreamData: &proxypb.StreamData{ Payload: payload, @@ -572,7 +589,8 @@ func TestWithFakeServerForErrors(t *testing.T) { }, }, }) - 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 diff --git a/proxy/server/server_test.go b/proxy/server/server_test.go index e96af6c3..879b8c6d 100644 --- a/proxy/server/server_test.go +++ b/proxy/server/server_test.go @@ -51,7 +51,7 @@ func startTestProxyWithAuthz(ctx context.Context, t *testing.T, targets map[stri 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() diff --git a/proxy/server/target_test.go b/proxy/server/target_test.go index 635dcc18..20e93a15 100644 --- a/proxy/server/target_test.go +++ b/proxy/server/target_test.go @@ -170,7 +170,8 @@ func TestTargetStreamAddNonBlocking(t *testing.T) { MethodName: "/Testdata.TestService/TestUnary", } go func() { - ss.Add(ctx, req, replyChan, nil) + err := ss.Add(ctx, req, replyChan, nil) + testutil.FatalOnErr("ss.Add", err, t) close(doneChan) }() select { diff --git a/proxy/testutil/testutil.go b/proxy/testutil/testutil.go index 8230dcae..8ca6c555 100644 --- a/proxy/testutil/testutil.go +++ b/proxy/testutil/testutil.go @@ -152,9 +152,11 @@ func (e *EchoTestDataServer) TestServerStream(req *tdpb.TestRequest, stream tdpb return errors.New("error") } for i := 0; i < 5; i++ { - stream.Send(&tdpb.TestResponse{ + if err := stream.Send(&tdpb.TestResponse{ Output: fmt.Sprintf("%s %d %s", e.serverName, i, req.Input), - }) + }); err != nil { + return err + } } return nil } @@ -272,7 +274,7 @@ func StartTestDataServer(t *testing.T, serverName string) *bufconn.Listener { 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. - rpcServer.Serve(lis) + _ = rpcServer.Serve(lis) }() t.Cleanup(func() { rpcServer.Stop() diff --git a/services/fdb/client/client.go b/services/fdb/client/client.go index 1fa07155..5102a88b 100644 --- a/services/fdb/client/client.go +++ b/services/fdb/client/client.go @@ -334,7 +334,10 @@ func (r *fdbCLICmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interf // We want this to still use the parser so move the string from --exec in as Args to the flagset f = flag.NewFlagSet("", flag.ContinueOnError) args := strings.Fields(r.exec) - f.Parse(args) + if err := f.Parse(args); err != nil { + fmt.Fprintf(os.Stderr, "command parse error: %v\n", err) + return subcommands.ExitFailure + } } c = setupFDBCLI(f) @@ -345,7 +348,10 @@ func (r *fdbCLICmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interf // A command of fdbcli "kill; kill X" will parse through here one by one. for _, a := range strings.Split(strings.Join(f.Args(), " "), ";") { a = strings.TrimSpace(a) - f.Parse(strings.Split(a, " ")) + if err := f.Parse(strings.Split(a, " ")); err != nil { + fmt.Fprintf(os.Stderr, "command parse error: %v\n", err) + return subcommands.ExitFailure + } exit := c.Execute(ctx, args...) if exit != subcommands.ExitSuccess { fmt.Fprintln(os.Stderr, "Error parsing command") diff --git a/services/fdb/server/conf_test.go b/services/fdb/server/conf_test.go index fef3411c..9fdd457d 100644 --- a/services/fdb/server/conf_test.go +++ b/services/fdb/server/conf_test.go @@ -77,9 +77,10 @@ func TestWrite(t *testing.T) { temp := t.TempDir() f1, err := os.CreateTemp(temp, "testfile.*") testutil.FatalOnErr("can't create tmpfile", err, t) - f1.WriteString(` + _, err = f1.WriteString(` [general] cluster_file = /etc/foundatindb/fdb.cluster`) + testutil.FatalOnErr("WriteString", err, t) name := f1.Name() err = f1.Close() testutil.FatalOnErr("can't close tmpfile", err, t) @@ -143,7 +144,7 @@ func TestDelete(t *testing.T) { temp := t.TempDir() f1, err := os.CreateTemp(temp, "testfile.*") testutil.FatalOnErr("can't create tmpfile", err, t) - f1.WriteString(`[general] + _, err = f1.WriteString(`[general] cluster_file = /etc/foundatindb/fdb.cluster [foo.1] @@ -151,6 +152,7 @@ bar = baz [foo.2] bar = baz`) + testutil.FatalOnErr("WriteString", err, t) name := f1.Name() err = f1.Close() testutil.FatalOnErr("can't close tmpfile", err, t) diff --git a/services/fdb/server/fdbcli_test.go b/services/fdb/server/fdbcli_test.go index b7d87d61..d574cbef 100644 --- a/services/fdb/server/fdbcli_test.go +++ b/services/fdb/server/fdbcli_test.go @@ -40,7 +40,7 @@ type logDef struct { perms fs.FileMode } -func fixupLogs(logs []captureLogs, def logDef) []captureLogs { +func fixupLogs(logs []captureLogs, def logDef) ([]captureLogs, error) { // Replace each log with our own pattern instead and generate logs so the RPC // can process them. for i := range logs { @@ -49,15 +49,21 @@ func fixupLogs(logs []captureLogs, def logDef) []captureLogs { if logs[i].IsDir { logs[i].Path = path.Join(def.basePath, def.subdir) // Create one with a suffix and one without. - os.WriteFile(path.Join(logs[i].Path, "file"), def.contents, def.perms) + if err := os.WriteFile(path.Join(logs[i].Path, "file"), def.contents, def.perms); err != nil { + return nil, err + } if logs[i].Suffix != "" { - os.WriteFile(path.Join(logs[i].Path, "file"+logs[i].Suffix), def.contents, def.perms) + if err := os.WriteFile(path.Join(logs[i].Path, "file"+logs[i].Suffix), def.contents, def.perms); err != nil { + return nil, err + } } continue } - os.WriteFile(fp, def.contents, def.perms) + if err := os.WriteFile(fp, def.contents, def.perms); err != nil { + return nil, err + } } - return logs + return logs, nil } func TestFDBCLI(t *testing.T) { @@ -3578,12 +3584,19 @@ func TestFDBCLI(t *testing.T) { generateFDBCLIArgs = func(req *pb.FDBCLIRequest) ([]string, []captureLogs, error) { generatedOpts, logs, err = origGen(req) - logs = fixupLogs(logs, logDef{ + var logErr error + logs, logErr = fixupLogs(logs, logDef{ basePath: temp, subdir: tc.subdir, contents: contents, perms: tc.perms, }) + if logErr != nil { + // Can't error out here since this is executing in the server thread and that won't + // roll up to our test thread. + t.Logf("error from fixupLogs: %v", logErr) + err = logErr + } if tc.ignoreOpts { return []string{tc.bin}, logs, err } diff --git a/services/localfile/client/client.go b/services/localfile/client/client.go index 5a4770db..e388dc71 100644 --- a/services/localfile/client/client.go +++ b/services/localfile/client/client.go @@ -249,27 +249,35 @@ func (s *statCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interfac return subcommands.ExitFailure } - waitc := make(chan error) + waitc := make(chan subcommands.ExitStatus) // Push all the sends into their own routine so we're receiving replies // at the same time. This way we can't deadlock if we send so much this // blocks which makes the server block its sends waiting on us to receive. go func() { - var err error + e := subcommands.ExitSuccess for _, filename := range f.Args() { - if err = stream.Send(&pb.StatRequest{Filename: filename}); err != nil { + if err := stream.Send(&pb.StatRequest{Filename: filename}); err != nil { // Emit this to every error file as it's not specific to a given target. for _, e := range state.Err { fmt.Fprintf(e, "All targets - stat: send error: %v\n", err) } + e = subcommands.ExitFailure break } } // Close the sending stream to notify the server not to expect any further data. // We'll process below but this let's the server politely know we're done sending // as otherwise it'll see this as a cancellation. - stream.CloseSend() - waitc <- err + if err := stream.CloseSend(); err != nil { + // Emit this to every error file as it's not specific to a given target. + for _, e := range state.Err { + fmt.Fprintf(e, "All targets - stat: CloseSend error: %v\n", err) + } + e = subcommands.ExitFailure + } + + waitc <- e close(waitc) }() @@ -314,10 +322,8 @@ func (s *statCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interfac fmt.Fprintf(state.Out[r.Index], outTmpl, r.Resp.Filename, r.Resp.Size, fileTypeString(mode), mode, r.Resp.Uid, r.Resp.Gid, r.Resp.Modtime.AsTime(), r.Resp.Immutable) } } - for err := range waitc { - if err != nil { - retCode = subcommands.ExitFailure - } + for e := range waitc { + retCode = e } return retCode @@ -398,27 +404,34 @@ func (s *sumCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interface return subcommands.ExitFailure } - waitc := make(chan error) + waitc := make(chan subcommands.ExitStatus) // Push all the sends into their own routine so we're receiving replies // at the same time. This way we can't deadlock if we send so much this // blocks which makes the server block its sends waiting on us to receive. go func() { - var err error + e := subcommands.ExitSuccess for _, filename := range f.Args() { - if err = stream.Send(&pb.SumRequest{Filename: filename, SumType: sumType}); err != nil { + if err := stream.Send(&pb.SumRequest{Filename: filename, SumType: sumType}); err != nil { // Emit this to every error file as it's not specific to a given target. for _, e := range state.Err { fmt.Fprintf(e, "All targets - sum: send error: %v\n", err) } + e = subcommands.ExitFailure break } } // Close the sending stream to notify the server not to expect any further data. // We'll process below but this let's the server politely know we're done sending // as otherwise it'll see this as a cancellation. - stream.CloseSend() - waitc <- err + if err := stream.CloseSend(); err != nil { + // Emit this to every error file as it's not specific to a given target. + for _, e := range state.Err { + fmt.Fprintf(e, "All targets - sum: CloseSend error: %v\n", err) + } + e = subcommands.ExitFailure + } + waitc <- e close(waitc) }() @@ -462,10 +475,8 @@ func (s *sumCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interface } } - for err := range waitc { - if err != nil { - retCode = subcommands.ExitFailure - } + for e := range waitc { + retCode = e } return retCode } diff --git a/services/localfile/server/localfile_darwin_test.go b/services/localfile/server/localfile_darwin_test.go index 8565f148..afabd4a8 100644 --- a/services/localfile/server/localfile_darwin_test.go +++ b/services/localfile/server/localfile_darwin_test.go @@ -43,7 +43,8 @@ func TestTailDarwin(t *testing.T) { f1, err := os.CreateTemp(temp, "testfile.*") testutil.FatalOnErr("can't create tmpfile", err, t) data := "Some data\n" - f1.WriteString(data) + _, err = f1.WriteString(data) + testutil.FatalOnErr("WriteString", err, t) name := f1.Name() err = f1.Close() testutil.FatalOnErr("closing file", err, t) diff --git a/services/localfile/server/localfile_linux_test.go b/services/localfile/server/localfile_linux_test.go index 7c500b3b..d3589ccd 100644 --- a/services/localfile/server/localfile_linux_test.go +++ b/services/localfile/server/localfile_linux_test.go @@ -43,7 +43,8 @@ func TestTailLinux(t *testing.T) { f1, err := os.CreateTemp(temp, "testfile.*") testutil.FatalOnErr("can't create tmpfile", err, t) data := "Some data\n" - f1.WriteString(data) + _, err = f1.WriteString(data) + testutil.FatalOnErr("WriteString", err, t) name := f1.Name() err = f1.Close() testutil.FatalOnErr("closing file", err, t) diff --git a/services/localfile/server/localfile_test.go b/services/localfile/server/localfile_test.go index 7af61c14..57c07c3b 100644 --- a/services/localfile/server/localfile_test.go +++ b/services/localfile/server/localfile_test.go @@ -214,7 +214,8 @@ func TestTail(t *testing.T) { f1, err := os.CreateTemp(temp, "testfile.*") testutil.FatalOnErr("can't create tmpfile", err, t) data := "Some data\n" - f1.WriteString(data) + _, err = f1.WriteString(data) + testutil.FatalOnErr("WriteString", err, t) name := f1.Name() err = f1.Close() testutil.FatalOnErr("closing file", err, t) @@ -522,7 +523,8 @@ func TestSetFileAttributes(t *testing.T) { // Construct a directory with no perms. We'll put // a file in there which should make chmod fail on it. badDir := filepath.Join(t.TempDir(), "/foo") - os.Mkdir(badDir, fs.ModePerm) + err = os.Mkdir(badDir, fs.ModePerm) + testutil.FatalOnErr("os.Mkdir", err, t) f2, err := os.CreateTemp(badDir, "testfile.*") testutil.FatalOnErr("os.CreateTemp", err, t) err = unix.Chmod(badDir, 0) @@ -562,7 +564,8 @@ func TestSetFileAttributes(t *testing.T) { chown = savedChown changeImmutableOS = savedChangeImmutableOS // Needed or we panic with generated cleanup trying to remove tmp directories. - unix.Chmod(badDir, uint32(fs.ModePerm)) + err = unix.Chmod(badDir, uint32(fs.ModePerm)) + testutil.FatalOnErr("Chmod", err, t) }) nobody, err := user.Lookup("nobody") @@ -912,7 +915,8 @@ func TestList(t *testing.T) { // Construct a directory with no perms. We should be able // to stat this but then fail to readdir on it. badDir := filepath.Join(t.TempDir(), "/foo") - os.Mkdir(badDir, 0) + err = os.Mkdir(badDir, 0) + testutil.FatalOnErr("Mkdir", err, t) origOsStat := osStat @@ -1709,7 +1713,8 @@ func TestRm(t *testing.T) { t.Cleanup(func() { // Needed or we panic with generated cleanup trying to remove tmp directories. - unix.Chmod(badDir, uint32(fs.ModePerm)) + err = unix.Chmod(badDir, uint32(fs.ModePerm)) + testutil.FatalOnErr("Chmod", err, t) }) for _, tc := range []struct { @@ -1762,7 +1767,8 @@ func TestRmdir(t *testing.T) { t.Cleanup(func() { // Needed or we panic with generated cleanup trying to remove tmp directories. - unix.Chmod(badDir, uint32(fs.ModePerm)) + err = unix.Chmod(badDir, uint32(fs.ModePerm)) + testutil.FatalOnErr("Chmod", err, t) }) for _, tc := range []struct { @@ -1810,7 +1816,8 @@ func TestRename(t *testing.T) { t.Cleanup(func() { // Needed or we panic with generated cleanup trying to remove tmp directories. - unix.Chmod(badDir, uint32(fs.ModePerm)) + err = unix.Chmod(badDir, uint32(fs.ModePerm)) + testutil.FatalOnErr("Chmod", err, t) }) for _, tc := range []struct { @@ -1858,10 +1865,11 @@ func TestRename(t *testing.T) { } { tc := tc t.Run(tc.name, func(t *testing.T) { - os.WriteFile(path.Join(temp, "file"), []byte("contents"), 0644) + err := os.WriteFile(path.Join(temp, "file"), []byte("contents"), 0644) + testutil.FatalOnErr("WriteFile", err, t) defer os.Remove(path.Join(temp, "file")) client := pb.NewLocalFileClient(conn) - _, err := client.Rename(ctx, tc.req) + _, err = client.Rename(ctx, tc.req) testutil.WantErr(tc.name, err, tc.wantErr, t) if tc.wantErr { return diff --git a/testing/integrate.sh b/testing/integrate.sh index a66bc90d..4bc16b13 100755 --- a/testing/integrate.sh +++ b/testing/integrate.sh @@ -333,29 +333,16 @@ if [ "${broke}" == "true" ]; then check_status 1 /dev/null Files missing license fi -echo -echo "Checking formatting" -echo -find . -type f -name \*.go | xargs gofmt -l >${LOGS}/gofmt -check_status $? /dev/null gofmt -if [ -s ${LOGS}/gofmt ]; then - cat ${LOGS}/gofmt - echo - check_status 1 /dev/null "Files listed need gofmt run on them" -fi - echo echo "Checking with go vet" echo go vet ./... check_status $? /dev/null go vet -# Build everything (this won't rebuild the binaries but the generate will) +# Build binaries including support for version checks echo echo "Running builds" echo -go build -v ./... -check_status $? /dev/null build go build -ldflags="-X github.com/Snowflake-Labs/sansshell/services/sansshell/server.Version=2p" -o bin/proxy-server ./cmd/proxy-server check_status $? /dev/null build proxy go build -o bin/sanssh ./cmd/sanssh