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

Fixup staticcheck and stylecheck, and violations thereof #5897

Merged
merged 10 commits into from Jan 21, 2022
Merged

Conversation

aarongable
Copy link
Contributor

Add stylecheck to our list of lints, since it got separated out from
staticcheck. Fix the way we configure both to be clearer and not
rely on regexes.

Additionally fix a number of easy-to-change staticcheck and
stylecheck violations, allowing us to reduce our number of ignored
checks.

Part of #5681

@aarongable aarongable requested a review from a team as a code owner January 19, 2022 20:21
@@ -185,7 +185,7 @@ func (va *ValidationAuthorityImpl) validateTLSALPN01(ctx context.Context, identi
return validationRecords, problem
}

if !cs.NegotiatedProtocolIsMutual || cs.NegotiatedProtocol != ACMETLS1Protocol {
if cs.NegotiatedProtocol != ACMETLS1Protocol {
Copy link
Contributor

Choose a reason for hiding this comment

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

Reference for anyone checking this diff: https://pkg.go.dev/crypto/tls#ConnectionState

// NegotiatedProtocolIsMutual used to indicate a mutual NPN negotiation.
//
// Deprecated: this value is always true.
NegotiatedProtocolIsMutual bool

@@ -166,7 +166,7 @@ func (ci *clientInterceptor) intercept(
defer cancel()
// Disable fail-fast so RPCs will retry until deadline, even if all backends
// are down.
opts = append(opts, grpc.FailFast(false))
opts = append(opts, grpc.WaitForReady(true))
Copy link
Contributor

Choose a reason for hiding this comment

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

Reference:

https://pkg.go.dev/google.golang.org/grpc#FailFast

FailFast is the opposite of WaitForReady.
Deprecated: use WaitForReady.

https://pkg.go.dev/google.golang.org/grpc#WaitForReady

WaitForReady configures the action to take when an RPC is attempted on broken connections or unreachable servers. If waitForReady is false and the connection is in the TRANSIENT_FAILURE state, the RPC will fail immediately. Otherwise, the RPC client will block the call until a connection is available (or the call is canceled or times out) and will retry the call if it fails due to a transient error. gRPC will not retry if data was written to the wire unless the server indicates it did not process the data. Please refer to https://github.com/grpc/grpc/blob/master/doc/wait-for-ready.md.

By default, RPCs don't "wait for ready".

}
}

unwrappedErr := status.Convert(err).Message()
Copy link
Contributor

Choose a reason for hiding this comment

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

Reference: https://pkg.go.dev/google.golang.org/grpc#ErrorDesc

ErrorDesc returns the error description of err if it was produced by the rpc system. Otherwise, it returns err.Error() or empty string when err is nil.
Deprecated: use status.Convert and Message method instead.

jsha
jsha previously approved these changes Jan 19, 2022
Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Reminder to other reviewers: when there are a lot of indentation diffs (like in errors.go) you can add ?w=1 to the URL to ignore whitespace when diffing.

@aarongable
Copy link
Contributor Author

I actually plan to add a little bit more to this -- these are two other checks (gosimple and unused) that staticcheck go broken out into. I have a PR to add them but I'd like to land #5898 first so that I don't have to update wfe1 code to pass the checks :) If you're reviewing this one, go review that one first!

@aarongable
Copy link
Contributor Author

Okay, I've got the additional lint checks re-added here. Please take a look!

@aarongable aarongable requested review from a team and jsha January 20, 2022 19:00
@aarongable aarongable merged commit ab79f96 into main Jan 21, 2022
@aarongable aarongable deleted the staticcheck branch January 21, 2022 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants