ci,lint: explicit linter configuration and subsequent fixes#141
Conversation
|
|
||
| defer conn.Close() | ||
| defer func() { | ||
| _ = conn.Close() |
There was a problem hiding this comment.
We can be stricter but this is just an example so 🤷🏻♂️
| defer proxyListener.Close() | ||
| defer func() { | ||
| if err := proxyListener.Close(); err != nil { | ||
| log.Printf("failed to close proxy listener: %v", err) |
There was a problem hiding this comment.
In this example, we actually log the error vs ignoring it like we did for client/client.go.
| } | ||
| if max := 1 * time.Second; delay > max { | ||
| delay = max | ||
| if maxDelay := 1 * time.Second; delay > maxDelay { |
There was a problem hiding this comment.
max is now a built-in function so we should not use it as a var name.
|
|
||
| go func(conn net.Conn, baseCtx context.Context) { | ||
| if err := srv.serveConn(conn, baseCtx); err != nil { | ||
| go func(baseCtx context.Context, conn net.Conn) { |
There was a problem hiding this comment.
context.Context ideally is the first param on a func's signature.
| } | ||
|
|
||
| // SSLType is true if the TLV is type SSL | ||
| // IsSSL reports whether the TLV is of SSL type. |
| } | ||
|
|
||
| // SSL returns the first PP2SSL if it exists and is well formed as well as bool indicating if it was found. | ||
| // FindSSL returns the first PP2SSL if it exists and is well formed. |
| - misspell | ||
| - revive | ||
| - unconvert | ||
| - usestdlibvars |
There was a problem hiding this comment.
Selected the ones I consider add up to the quality of the project from a security and readability perspectives.
| allow-parallel-runners: true | ||
|
|
||
| issues: | ||
| max-issues-per-linter: 0 |
There was a problem hiding this comment.
Default is 50 which makes it harder to massively fix linter errors.
| // can't decide the policy, we can't accept the connection. | ||
| if closeErr := conn.Close(); closeErr != nil { | ||
| return nil, closeErr | ||
| } |
There was a problem hiding this comment.
If we must close the connection then we must error out if we are unable to, right?
| if sourcePort, destPort, ok := header.Ports(); ok { | ||
| if sourcePort < 0 || sourcePort > math.MaxUint16 || destPort < 0 || destPort > math.MaxUint16 { | ||
| return nil, ErrInvalidPortNumber | ||
| } |
| func parseUnixName(b []byte) string { | ||
| i := bytes.IndexByte(b, 0) | ||
| if i < 0 { | ||
| before, _, ok := bytes.Cut(b, []byte{0}) |
There was a problem hiding this comment.
Added unit tests to prove it works as expected.
| // Make two calls to trigger the listener's accept, the first should experience | ||
| // the ErrInvalidUpstream and keep the listener open, the second should experience | ||
| // a different error which will cause the listener to close. | ||
| _, _ = http.Get("http://localhost:8080") |
There was a problem hiding this comment.
Made this test way more explicit to better fit the description.
| select { | ||
| case err := <-errCh: | ||
| if err == nil { | ||
| t.Fatalf("errors other than invalid upstream should error") |
There was a problem hiding this comment.
This seems wrong, doesn't it?
|
Sorry, I ended up not having enough time to review this PR. |
|
Totally get it. Thank you for taking the time, it's much appreciated. I guess we'll learn soon if things break :D |
3 participants
I realized I didn't have an explicit list of linters and the
defaultset is somewhat limited.@emersion I know it's too much to ask but I would sleep better at night if this wasn't just a me thing 😅 🙏🏻 I left comments where I think attention is asked for.