Skip to content

Commit

Permalink
Fix error checking to enable golint errcheck in CLI
Browse files Browse the repository at this point in the history
Summary:
This is one diff in a series of diffs to fix the error checking in our golang code so that we can ultimately enable golint's errcheck.
this lint rule requires errors to be handled, whether that is just a log or returning the error, etc.
this part mainly tackles the CLI.

Test Plan: unit tests pass, also ran CLI

Reviewers: zasgar, vihang, nserrino

Reviewed By: vihang

Differential Revision: https://phab.corp.pixielabs.ai/D7963

GitOrigin-RevId: 8e16396
  • Loading branch information
Michelle Nguyen committed Apr 5, 2021
1 parent affcf4f commit 9c26efa
Show file tree
Hide file tree
Showing 33 changed files with 197 additions and 53 deletions.
4 changes: 3 additions & 1 deletion .golangci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ linters:

linters-settings:
errcheck:
ignore: github.com/spf13/viper:BindPFlag
# yamllint disable
ignore: github.com/spf13/pflag:MarkHidden,github.com/spf13/viper:BindPFlag,github.com/spf13/cobra:(Help|MarkFlagRequired|Usage),gopkg.in/segmentio/analytics-go.v3:Enqueue,database/sql:Rollback,github.com/nats-io/stan.go:Unsubscribe,github.com/nats-io/nats.go:Unsubscribe
# yamllint enable
goimports:
local-prefixes: pixielabs.ai
nakedret:
Expand Down
2 changes: 1 addition & 1 deletion src/cloud/api/ptproxy/vizier_pt_proxy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,7 @@ func TestVizierPassThroughProxy_DebugLog(t *testing.T) {
}
}
})
eg.Wait()
err = eg.Wait()
if err != nil {
t.Fatal(err)
}
Expand Down
7 changes: 6 additions & 1 deletion src/cloud/vzmgr/controller/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -836,7 +836,12 @@ func (s *Server) HandleVizierHeartbeat(v2cMsg *cvmsgspb.V2CMessage) {
}

// If we reach this point, the cluster is in bootstrap mode and needs to be deployed.
go s.updater.UpdateOrInstallVizier(vizierID, req.BootstrapVersion, false)
go func() {
_, err := s.updater.UpdateOrInstallVizier(vizierID, req.BootstrapVersion, false)
if err != nil {
log.WithError(err).Error("Failed to update or install vizier")
}
}()
}

// HandleSSLRequest registers certs for the vizier cluster.
Expand Down
5 changes: 4 additions & 1 deletion src/e2e_test/cli/cli_e2e_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,10 @@ func cliExecStdoutBytes(t *testing.T, args ...string) (*bytes.Buffer, error) {

var buf bytes.Buffer
go func() {
io.Copy(&buf, r)
_, err := io.Copy(&buf, r)
if err != nil {
log.WithError(err).Error("Failed to copy")
}
}()

if err := c.Run(); err != nil {
Expand Down
16 changes: 13 additions & 3 deletions src/pixie_cli/pkg/auth/login.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@ func EnsureDefaultAuthFilePath() (string, error) {

pixieDirPath := filepath.Join(u.HomeDir, pixieAuthPath)
if _, err := os.Stat(pixieDirPath); os.IsNotExist(err) {
os.Mkdir(pixieDirPath, 0744)
err := os.Mkdir(pixieDirPath, 0744)
if err != nil {
return "", err
}
}

pixieAuthFilePath := filepath.Join(pixieDirPath, pixieAuthFile)
Expand Down Expand Up @@ -300,7 +303,11 @@ func (p *PixieCloudLogin) tryBrowserAuth() (*RefreshToken, error) {

ctx, cancel := context.WithTimeout(context.Background(), time.Minute*2)
defer cancel()
defer h.Shutdown(ctx)
defer func() {
err := h.Shutdown(ctx)
log.WithError(err).Error("Failed to shutdown server")
}()

for {
select {
case <-ctx.Done():
Expand Down Expand Up @@ -329,7 +336,10 @@ func (p *PixieCloudLogin) getAuthStringManually() (string, error) {
// fmt.Printf appears to escape % (as desired) so we use it here instead of the cli logger.
fmt.Printf("\nPlease Visit: \n \t %s\n\n", authURL.String())
f := bufio.NewWriter(os.Stdout)
f.WriteString("Copy and paste token here: ")
_, err := f.WriteString("Copy and paste token here: ")
if err != nil {
return "", err
}
f.Flush()

r := bufio.NewReader(os.Stdin)
Expand Down
6 changes: 5 additions & 1 deletion src/pixie_cli/pkg/cmd/demo.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,11 @@ func listCmd(cmd *cobra.Command, args []string) {
for app, appSpec := range manifest {
// When a demo app is deprecated, its contents will be set to null in manifest.json.
if appSpec != nil {
w.Write([]interface{}{app})
err = w.Write([]interface{}{app})
if err != nil {
log.WithError(err).Error("Failed to write demo app")
continue
}
}
}
}
Expand Down
7 changes: 6 additions & 1 deletion src/pixie_cli/pkg/cmd/deploy.go
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,12 @@ func runDeployCmd(cmd *cobra.Command, args []string) {
// Using log.Fatal rather than CLI log in order to track this unexpected error in Sentry.
log.WithError(err).Fatal("Failed to generate deployment key")
}
defer deleteDeployKey(cloudAddr, uuid.FromStringOrNil(deployKeyID))
defer func() {
err := deleteDeployKey(cloudAddr, uuid.FromStringOrNil(deployKeyID))
if err != nil {
log.WithError(err).Info("Failed to delete generated deploy key")
}
}()
}

kubeConfig := k8s.GetConfig()
Expand Down
1 change: 1 addition & 0 deletions src/pixie_cli/pkg/cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ func init() {
RootCmd.PersistentFlags().MarkHidden("cloud_addr")
}

// nolint:errcheck
func printTestingBanner() {
r := color.New(color.Bold, color.FgRed).Fprintf
r(os.Stderr, "*******************************\n")
Expand Down
5 changes: 4 additions & 1 deletion src/pixie_cli/pkg/cmd/script_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,10 @@ func listBundleScripts(br *script.BundleManager, format string) {
if script.Hidden {
continue
}
w.Write([]interface{}{script.ScriptName, script.ShortDoc})
err := w.Write([]interface{}{script.ScriptName, script.ShortDoc})
if err != nil {
log.WithError(err).Error("Failed to write to stream")
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/pixie_cli/pkg/live/live.go
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,7 @@ func (v *View) showScriptView() {
v.pages.AddAndSwitchToPage("script", tv, true)
if v.s.execScript != nil {
highlighted := strings.Builder{}
quick.Highlight(&highlighted, v.s.execScript.ScriptString, "python",
quick.Highlight(&highlighted, v.s.execScript.ScriptString, "python", // nolint: errcheck
"terminal16m", "monokai")
fmt.Fprintf(tv, "%s :\n\n", withAccent("Script View"))
fmt.Fprint(tv, tview.TranslateANSI(highlighted.String()))
Expand Down
5 changes: 4 additions & 1 deletion src/pixie_cli/pkg/live/new_autocomplete.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,10 @@ func (m *tabAutocompleteModal) parseCommand() (*script.ExecutableScript, error)
return nil, err
}
}
es.UpdateFlags(fs)
err = es.UpdateFlags(fs)
if err != nil {
return nil, err
}
return es, nil
}

Expand Down
5 changes: 4 additions & 1 deletion src/pixie_cli/pkg/live/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ func tryJSONHighlight(s string) string {
}

highlighted := strings.Builder{}
quick.Highlight(&highlighted, string(parsed), "json", "terminal16m", "monokai")
err = quick.Highlight(&highlighted, string(parsed), "json", "terminal16m", "monokai")
if err != nil {
return s
}
return highlighted.String()
}

Expand Down
5 changes: 4 additions & 1 deletion src/pixie_cli/pkg/pxconfig/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,10 @@ func ensureDefaultConfigFilePath() (string, error) {

pixieDirPath := filepath.Join(u.HomeDir, pixieDotPath)
if _, err := os.Stat(pixieDirPath); os.IsNotExist(err) {
os.Mkdir(pixieDirPath, 0744)
err = os.Mkdir(pixieDirPath, 0744)
if err != nil {
return "", err
}
}

pixieConfigFilePath := filepath.Join(pixieDirPath, pixieConfigFile)
Expand Down
3 changes: 2 additions & 1 deletion src/pixie_cli/pkg/script/flagset_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ func TestSetFlag(t *testing.T) {

assert.Nil(t, flags.Parse(flagVals))

flags.Set("f3", "555")
err := flags.Set("f3", "555")
require.NoError(t, err)

f3, err := flags.Lookup("f3")
require.NoError(t, err)
Expand Down
5 changes: 4 additions & 1 deletion src/pixie_cli/pkg/vizier/data_formatter.go
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,10 @@ func (d *dataFormatterImpl) formatKV(valueDataType public_vizierapipb.DataType,
return toString(val)
}
var result map[string]interface{}
json.Unmarshal([]byte(strVal), &result)
err := json.Unmarshal([]byte(strVal), &result)
if err != nil {
return toString(val)
}

var keys []string
for k := range result {
Expand Down
20 changes: 16 additions & 4 deletions src/pixie_cli/pkg/vizier/script.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,10 @@ func RunScriptAndOutputResults(ctx context.Context, conns []*Connector, execScri

tw, err := runScript(ctx, conns, execScript, format)
if err == nil { // Script ran successfully.
tw.Finish()
err = tw.Finish()
if err != nil {
return err
}
return nil
}

Expand All @@ -62,7 +65,10 @@ func RunScriptAndOutputResults(ctx context.Context, conns []*Connector, execScri
mutationInfo, _ := tw.MutationInfo()
if mutationInfo == nil || (mutationInfo.Status.Code != int32(codes.Unavailable)) {
// There is no mutation in the script, or the mutation is complete.
tw.Finish()
err = tw.Finish()
if err != nil {
return err
}
return err
}

Expand Down Expand Up @@ -138,9 +144,15 @@ func RunScriptAndOutputResults(ctx context.Context, conns []*Connector, execScri
}
}()

vzJr.RunAndMonitor()
err = vzJr.RunAndMonitor()
if err != nil {
return err
}
if tw != nil {
tw.Finish()
err = tw.Finish()
if err != nil {
return err
}
}
return err
}
Expand Down
5 changes: 4 additions & 1 deletion src/shared/services/healthz/healthz.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,10 @@ func registerRootHealthzChecks(checks ...Checker) http.HandlerFunc {
}

fmt.Fprint(w, "OK\n")
verboseOut.WriteTo(w)
_, err := verboseOut.WriteTo(w)
if err != nil {
log.WithError(err).Error("Failed to write to response")
}
fmt.Fprint(w, "healthz check passed\n")
})
}
Expand Down
3 changes: 2 additions & 1 deletion src/shared/services/msgbus/nats_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ func TestMustConnectNATS(t *testing.T) {
ch := make(chan *nats.Msg)
_, err := nc.ChanSubscribe(sub, ch)
require.NoError(t, err)
nc.Publish(sub, msg)
err = nc.Publish(sub, msg)
require.NoError(t, err)
natsMsg := <-ch
assert.Equal(t, natsMsg.Data, msg)
}
7 changes: 5 additions & 2 deletions src/shared/services/msgbus/stan_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"github.com/nats-io/stan.go"
"github.com/spf13/viper"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"pixielabs.ai/pixielabs/src/shared/services/msgbus"
"pixielabs.ai/pixielabs/src/utils/testingutils"
Expand All @@ -24,10 +25,12 @@ func TestMustConnectSTAN(t *testing.T) {
sub := "abc"
data := []byte("123")
ch := make(chan *stan.Msg)
sc.Subscribe(sub, func(msg *stan.Msg) {
_, err := sc.Subscribe(sub, func(msg *stan.Msg) {
ch <- msg
})
sc.Publish(sub, data)
require.NoError(t, err)
err = sc.Publish(sub, data)
require.NoError(t, err)
msg := <-ch
assert.Equal(t, msg.Data, data)
}
5 changes: 4 additions & 1 deletion src/shared/services/pgtest/pgtest.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,10 @@ func SetupTestDB(schemaSource *bindata.AssetSource) (*sqlx.DB, func(), error) {
return nil, nil, fmt.Errorf("Failed to run docker pool: %w", err)
}
// Set a 5 minute expiration on resources.
resource.Expire(300)
err = resource.Expire(300)
if err != nil {
return nil, nil, err
}

viper.Set("postgres_port", resource.GetPort("5432/tcp"))
viper.Set("postgres_hostname", "localhost")
Expand Down
17 changes: 13 additions & 4 deletions src/shared/services/server/grpc_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,10 @@ func (s *testserver) Ping(ctx context.Context, in *ping.PingRequest) (*ping.Ping
}

func (s *testserver) PingServerStream(in *ping.PingRequest, srv ping.PingService_PingServerStreamServer) error {
srv.Send(&ping.PingReply{Reply: "test reply"})
err := srv.Send(&ping.PingReply{Reply: "test reply"})
if err != nil {
return err
}
return nil
}

Expand All @@ -48,7 +51,10 @@ func (s *testserver) PingClientStream(srv ping.PingService_PingClientStreamServe
if msg == nil {
return fmt.Errorf("Got a nil message")
}
srv.SendAndClose(&ping.PingReply{Reply: "test reply"})
err = srv.SendAndClose(&ping.PingReply{Reply: "test reply"})
if err != nil {
return err
}
return nil
}

Expand Down Expand Up @@ -104,12 +110,15 @@ func makeTestClientStreamRequest(ctx context.Context, t *testing.T, lis *bufconn
c := ping.NewPingServiceClient(conn)

stream, err := c.PingClientStream(ctx)
stream.Send(&ping.PingRequest{Req: "hello"})

if err != nil {
t.Fatalf("Could not create stream")
}

err = stream.Send(&ping.PingRequest{Req: "hello"})
if err != nil {
t.Fatalf("Could not send on stream")
}

return stream.CloseAndRecv()
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@ func sayHello(w http.ResponseWriter, r *http.Request) {
name = nameArgs[0]
}
reply := helloReply{Greeter: "Hello " + name + "!"}
json.NewEncoder(w).Encode(reply)
err := json.NewEncoder(w).Encode(reply)
if err != nil {
log.Fatal(err)
}
}

func main() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,10 @@ func bidirStreamGreet(address string, https bool, names []string) {
}
log.Println(reply.Message)
}
stream.CloseSend()
err = stream.CloseSend()
if err != nil {
log.Fatalf("Failed to close send")
}
}

func connectAndGreet(address string, https bool, name string) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@ func (s *server) SayHelloServerStreaming(in *pb.HelloRequest, srv pb.StreamingGr
// Send 3 responses each time. We do not care much about the exact number of responses, this is for executing the
// server streaming mechanism and observe the underlying HTTP2 framing data.
for i := 0; i < 3; i++ {
srv.Send(&pb.HelloReply{Message: "Hello " + in.Name})
err := srv.Send(&pb.HelloReply{Message: "Hello " + in.Name})
if err != nil {
return err
}
}
return nil
}
Expand All @@ -62,7 +65,10 @@ func (s *server) SayHelloBidirStreaming(stream pb.StreamingGreeter_SayHelloBidir
if err != nil {
return err
}
stream.Send(&pb.HelloReply{Message: "Hello " + helloReq.Name})
err = stream.Send(&pb.HelloReply{Message: "Hello " + helloReq.Name})
if err != nil {
return err
}
}
}

Expand Down
Loading

0 comments on commit 9c26efa

Please sign in to comment.