Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to golangci-lint v1.60.1 #570

Merged
merged 1 commit into from
Aug 20, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
@@ -82,7 +82,7 @@ check-protos: clean-protos protos ## Re-generates protos and git diffs them
GOPATH=$(CURDIR)/.tools go install github.com/fatih/faillint@v1.13.0

.tools/bin/golangci-lint: .tools
GOPATH=$(CURDIR)/.tools go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.59.1
GOPATH=$(CURDIR)/.tools go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.60.1

.tools/bin/protoc: .tools
ifeq ("$(wildcard .tools/protoc/bin/protoc)","")
13 changes: 7 additions & 6 deletions grpcutil/status_test.go
Original file line number Diff line number Diff line change
@@ -2,6 +2,7 @@ package grpcutil

import (
"context"
"errors"
"fmt"
"net/http"
"testing"
@@ -26,10 +27,10 @@ func TestErrorToStatus(t *testing.T) {
err: nil,
},
"a random error cannot be cast to status.Status": {
err: fmt.Errorf(msgErr),
err: errors.New(msgErr),
},
"a wrapped error of a random error cannot be cast to status.Status": {
err: fmt.Errorf("wrapped: %w", fmt.Errorf(msgErr)),
err: fmt.Errorf("wrapped: %w", errors.New(msgErr)),
},
"a gRPC error built by gogo/status can be cast to status.Status": {
err: status.Error(codes.Internal, msgErr),
@@ -74,11 +75,11 @@ func TestErrorToStatusCode(t *testing.T) {
expectedStatusCode: codes.OK,
},
"a non-gRPC error returns codes.Unknown": {
err: fmt.Errorf(msgErr),
err: errors.New(msgErr),
expectedStatusCode: codes.Unknown,
},
"a wrapped non-gRPC error returns codes.Unknown": {
err: fmt.Errorf("wrapped: %w", fmt.Errorf(msgErr)),
err: fmt.Errorf("wrapped: %w", errors.New(msgErr)),
expectedStatusCode: codes.Unknown,
},
"a gRPC error built by gogo/status returns its code": {
@@ -132,11 +133,11 @@ func TestIsCanceled(t *testing.T) {
expectedOutcome: true,
},
"a random error returns false": {
err: fmt.Errorf(msgErr),
err: errors.New(msgErr),
expectedOutcome: false,
},
"a wrapped random error returns false": {
err: fmt.Errorf("wrapped: %w", fmt.Errorf(msgErr)),
err: fmt.Errorf("wrapped: %w", errors.New(msgErr)),
expectedOutcome: false,
},
"a gRPC error with code different from codes.Canceled returns false": {
13 changes: 10 additions & 3 deletions httpgrpc/httpgrpc.go
Original file line number Diff line number Diff line change
@@ -106,16 +106,23 @@ func FromHeader(hs http.Header) []*Header {
return result
}

// Errorf returns a HTTP gRPC error than is correctly forwarded over
// Error returns a HTTP gRPC error that is correctly forwarded over
// gRPC, and can eventually be converted back to a HTTP response with
// HTTPResponseFromError.
func Errorf(code int, tmpl string, args ...interface{}) error {
func Error(code int, msg string) error {
return ErrorFromHTTPResponse(&HTTPResponse{
Code: int32(code),
Body: []byte(fmt.Sprintf(tmpl, args...)),
Body: []byte(msg),
})
}

// Errorf returns a HTTP gRPC error that is correctly forwarded over
// gRPC, and can eventually be converted back to a HTTP response with
// HTTPResponseFromError.
func Errorf(code int, tmpl string, args ...interface{}) error {
return Error(code, fmt.Sprintf(tmpl, args...))
}

// ErrorFromHTTPResponse converts an HTTP response into a grpc error, and uses HTTP response body as an error message.
// Note that if HTTP response body contains non-utf8 string, then returned error cannot be marshalled by protobuf.
func ErrorFromHTTPResponse(resp *HTTPResponse) error {
9 changes: 5 additions & 4 deletions httpgrpc/httpgrpc_test.go
Original file line number Diff line number Diff line change
@@ -2,6 +2,7 @@ package httpgrpc

import (
"context"
"errors"
"fmt"
"net/http"
"testing"
@@ -52,7 +53,7 @@ func TestErrorf(t *testing.T) {
Code: int32(code),
Body: []byte(errMsg),
}
err := Errorf(code, errMsg)
err := Error(code, errMsg)
stat, ok := status.FromError(err)
require.True(t, ok)
require.Equal(t, code, int(stat.Code()))
@@ -90,7 +91,7 @@ func TestHTTPResponseFromError(t *testing.T) {
err: nil,
},
"a random error cannot be parsed to an HTTPResponse": {
err: fmt.Errorf(msgErr),
err: errors.New(msgErr),
},
"a gRPC error built by gogo/status cannot be parsed to an HTTPResponse": {
err: status.Error(codes.Internal, msgErr),
@@ -99,11 +100,11 @@ func TestHTTPResponseFromError(t *testing.T) {
err: grpcstatus.Error(codes.Internal, msgErr),
},
"a gRPC error built by httpgrpc can be parsed to an HTTPResponse": {
err: Errorf(400, msgErr),
err: Error(400, msgErr),
expectedHTTPResponse: &HTTPResponse{Code: 400, Body: []byte(msgErr)},
},
"a wrapped gRPC error built by httpgrpc can be parsed to an HTTPResponse": {
err: fmt.Errorf("wrapped: %w", Errorf(400, msgErr)),
err: fmt.Errorf("wrapped: %w", Error(400, msgErr)),
expectedHTTPResponse: &HTTPResponse{Code: 400, Body: []byte(msgErr)},
},
}
6 changes: 3 additions & 3 deletions middleware/grpc_instrumentation_test.go
Original file line number Diff line number Diff line change
@@ -219,11 +219,11 @@ func TestInstrumentationLabel_ErrorToStatusCode(t *testing.T) {
expectedGRPCStatueCodes: codes.FailedPrecondition,
},
"a gRPC error with status codes.Canceled returns codes.Canceled": {
err: status.Errorf(codes.Canceled, context.Canceled.Error()),
err: status.Error(codes.Canceled, context.Canceled.Error()),
expectedGRPCStatueCodes: codes.Canceled,
},
"a wrapped gRPC error with status codes.Canceled returns codes.Canceled": {
err: fmt.Errorf("wrapped: %w", status.Errorf(codes.Canceled, context.Canceled.Error())),
err: fmt.Errorf("wrapped: %w", status.Error(codes.Canceled, context.Canceled.Error())),
expectedGRPCStatueCodes: codes.Canceled,
},
"context.Canceled returns codes.Canceled": {
@@ -239,7 +239,7 @@ func TestInstrumentationLabel_ErrorToStatusCode(t *testing.T) {
expectedGRPCStatueCodes: codes.Unknown,
},
"a non-gRPC error returns codes.Unknown": {
err: fmt.Errorf(errMsg),
err: errors.New(errMsg),
expectedGRPCStatueCodes: codes.Unknown,
},
}
2 changes: 1 addition & 1 deletion ring/replication_set.go
Original file line number Diff line number Diff line change
@@ -316,7 +316,7 @@ func DoUntilQuorumWithoutSuccessfulContextCancellation[T any](ctx context.Contex
ext.Error.Set(cfg.Logger.Span, true)
}

contextTracker.cancelAllContexts(cancellation.NewErrorf(cause))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see why the new version is better.

Copy link
Collaborator Author

@aknuds1 aknuds1 Aug 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's generally because the linter thinks the non-formatting version should be used, if there's no string formatting. Why do you eventually think the linter is wrong about this?

contextTracker.cancelAllContexts(cancellation.NewError(errors.New(cause)))
cleanupResultsAlreadyReceived()
return nil, err
}
3 changes: 1 addition & 2 deletions server/server_test.go
Original file line number Diff line number Diff line change
@@ -18,7 +18,6 @@ import (
"os"
"os/exec"
"path/filepath"
"strconv"
"testing"
"time"

@@ -48,7 +47,7 @@ func (f FakeServer) FailWithError(_ context.Context, _ *protobuf.Empty) (*protob
}

func (f FakeServer) FailWithHTTPError(_ context.Context, req *FailWithHTTPErrorRequest) (*protobuf.Empty, error) {
return nil, httpgrpc.Errorf(int(req.Code), strconv.Itoa(int(req.Code)))
return nil, httpgrpc.Errorf(int(req.Code), "%d", req.Code)
}

func (f FakeServer) Succeed(_ context.Context, _ *protobuf.Empty) (*protobuf.Empty, error) {