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

Add staticcheck github action, resolve existing lint #254

Merged
merged 16 commits into from
Feb 29, 2024
Merged
Show file tree
Hide file tree
Changes from 15 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
26 changes: 26 additions & 0 deletions .github/workflows/static-check.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
name: static-check

on:
pull_request:
push:
tags:
- 'v*'
branches:
- main

jobs:
staticcheck:
name: staticcheck (project)
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v1
with:
fetch-depth: 1
- id: install_go
uses: WillAbides/[email protected]
with:
go-version: "1.20.x"
- uses: dominikh/[email protected]
with:
install-go: false
version: "2023.1.7"
20 changes: 12 additions & 8 deletions client/httpclient_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package client
import (
"compress/gzip"
"context"
"io/ioutil"
"io"
"net/http"
"sync/atomic"
"testing"
Expand All @@ -22,10 +22,12 @@ func TestHTTPPolling(t *testing.T) {
srv := internal.StartMockServer(t)
var rcvCounter int64
srv.OnMessage = func(msg *protobufs.AgentToServer) *protobufs.ServerToAgent {
assert.EqualValues(t, rcvCounter, msg.SequenceNum)
if msg != nil {
atomic.AddInt64(&rcvCounter, 1)
if msg == nil {
t.Error("unexpected nil msg")
return nil
echlebek marked this conversation as resolved.
Show resolved Hide resolved
}
assert.EqualValues(t, rcvCounter, msg.SequenceNum)
atomic.AddInt64(&rcvCounter, 1)
return nil
}

Expand Down Expand Up @@ -63,7 +65,7 @@ func TestHTTPClientCompression(t *testing.T) {
assert.Equal(t, "gzip", r.Header.Get("Content-Encoding"))
reader, err := gzip.NewReader(r.Body)
assert.NoError(t, err)
body, err := ioutil.ReadAll(reader)
body, err := io.ReadAll(reader)
assert.NoError(t, err)
_ = r.Body.Close()
var response protobufs.AgentToServer
Expand Down Expand Up @@ -112,10 +114,12 @@ func TestHTTPClientSetPollingInterval(t *testing.T) {
srv := internal.StartMockServer(t)
var rcvCounter int64
srv.OnMessage = func(msg *protobufs.AgentToServer) *protobufs.ServerToAgent {
assert.EqualValues(t, rcvCounter, msg.SequenceNum)
if msg != nil {
atomic.AddInt64(&rcvCounter, 1)
if msg == nil {
t.Error("unexpected nil msg")
return nil
}
assert.EqualValues(t, rcvCounter, msg.SequenceNum)
atomic.AddInt64(&rcvCounter, 1)
return nil
}

Expand Down
1 change: 0 additions & 1 deletion client/internal/httpsender.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,6 @@ func (h *HTTPSender) Run(
// This will make hasPendingMessage channel readable, so we will enter
// the case above on the next iteration of the loop.
h.ScheduleSend()
break

case <-ctx.Done():
return
Expand Down
9 changes: 7 additions & 2 deletions client/internal/mockserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@
func newMockServer(t *testing.T) (*MockServer, *http.ServeMux) {
srv := &MockServer{
t: t,
expectedHandlers: make(chan receivedMessageHandler, 0),
expectedComplete: make(chan struct{}, 0),
expectedHandlers: make(chan receivedMessageHandler),
expectedComplete: make(chan struct{}),
}

m := http.NewServeMux()
Expand Down Expand Up @@ -112,6 +112,11 @@

func (m *MockServer) handlePlainHttp(w http.ResponseWriter, r *http.Request) {
msgBytes, err := io.ReadAll(r.Body)
if err != nil {
// request could not be read for some reason
http.Error(w, err.Error(), 500)
tigrannajaryan marked this conversation as resolved.
Show resolved Hide resolved
return

Check warning on line 118 in client/internal/mockserver.go

View check run for this annotation

Codecov / codecov/patch

client/internal/mockserver.go#L117-L118

Added lines #L117 - L118 were not covered by tests
}

// We use alwaysRespond=true here because plain HTTP requests must always have
// a response.
Expand Down
4 changes: 2 additions & 2 deletions client/internal/packagessyncer.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func (s *packagesSyncer) doSync(ctx context.Context) {
s.logger.Errorf(ctx, "Package syncing failed: %V", err)
return
}
if bytes.Compare(hash, s.available.AllPackagesHash) == 0 {
if bytes.Equal(hash, s.available.AllPackagesHash) {
s.logger.Debugf(ctx, "All packages are already up to date.")
return
}
Expand Down Expand Up @@ -243,7 +243,7 @@ func (s *packagesSyncer) shouldDownloadFile(ctx context.Context, packageName str
} else {
// Compare the checksum of the file we have with what
// we are offered by the server.
if bytes.Compare(fileContentHash, file.ContentHash) != 0 {
if !bytes.Equal(fileContentHash, file.ContentHash) {
s.logger.Debugf(ctx, "Package %s: file hash mismatch, will download.", packageName)
return true, nil
}
Expand Down
12 changes: 6 additions & 6 deletions client/wsclient.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ func (c *wsClient) SetPackageStatuses(statuses *protobufs.PackageStatuses) error
// Try to connect once. Returns an error if connection fails and optional retryAfter
// duration to indicate to the caller to retry after the specified time as instructed
// by the Server.
func (c *wsClient) tryConnectOnce(ctx context.Context) (err error, retryAfter sharedinternal.OptionalDuration) {
func (c *wsClient) tryConnectOnce(ctx context.Context) (retryAfter sharedinternal.OptionalDuration, err error) {
var resp *http.Response
conn, resp, err := c.dialer.DialContext(ctx, c.url.String(), c.requestHeader)
if err != nil {
Expand All @@ -142,7 +142,7 @@ func (c *wsClient) tryConnectOnce(ctx context.Context) (err error, retryAfter sh
redirect, err := resp.Location()
if err != nil {
c.common.Logger.Errorf(ctx, "%d redirect, but no valid location: %s", resp.StatusCode, err)
return err, duration
return duration, err
}
// rewrite the scheme for the sake of tolerance
if redirect.Scheme == "http" {
Expand All @@ -157,9 +157,9 @@ func (c *wsClient) tryConnectOnce(ctx context.Context) (err error, retryAfter sh
} else {
c.common.Logger.Errorf(ctx, "Server responded with status=%v", resp.Status)
}
return err, duration
return duration, err
}
return err, sharedinternal.OptionalDuration{Defined: false}
return sharedinternal.OptionalDuration{Defined: false}, err
}

// Successfully connected.
Expand All @@ -170,7 +170,7 @@ func (c *wsClient) tryConnectOnce(ctx context.Context) (err error, retryAfter sh
c.common.Callbacks.OnConnect(ctx)
}

return nil, sharedinternal.OptionalDuration{Defined: false}
return sharedinternal.OptionalDuration{Defined: false}, nil
}

// Continuously try until connected. Will return nil when successfully
Expand All @@ -190,7 +190,7 @@ func (c *wsClient) ensureConnected(ctx context.Context) error {
select {
case <-timer.C:
{
if err, retryAfter := c.tryConnectOnce(ctx); err != nil {
if retryAfter, err := c.tryConnectOnce(ctx); err != nil {
c.lastInternalErr.Store(&err)
if errors.Is(err, context.Canceled) {
c.common.Logger.Debugf(ctx, "Client is stopped, will not try anymore.")
Expand Down
6 changes: 2 additions & 4 deletions internal/certs.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
"encoding/pem"
"errors"
"fmt"
"io/ioutil"
"math/big"
"net"
"os"
Expand Down Expand Up @@ -79,19 +78,18 @@
InsecureSkipVerify: true,
ClientCAs: caCertPool,
}
tlsConfig.BuildNameToCertificate()
Copy link
Member

Choose a reason for hiding this comment

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

Reading the deprecation notice this seems to be fine, but I am not entirely sure since the notice is a bit vague.
Are you able to verify that the example server that uses this functionality still works fine? The clients should be able to connect if everything is working correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can add a test with a TLS server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thoughts on using #202?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah #202 will need to be updated, it has conflicts. I don't think you need to add a test as part of this particular PR. If you can confirm by running the examples manually that they still work we should be good and can merge this PR.

Copy link
Member

Choose a reason for hiding this comment

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

make run-examples should build and run the example server and agent and if you go to http://localhost:4321/ you should be able to see a connected agent.

image

Click on it, scroll down and ensure you see it connected using a client certificate, e.g.:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Verified that the client certificate is shown on this page. Thanks for the instructions!

image

return tlsConfig, nil
}

func CreateTLSCert(caCertPath, caKeyPath string) (*protobufs.TLSCertificate, error) {

// Load CA Cert.
caCertBytes, err := ioutil.ReadFile(caCertPath)
caCertBytes, err := os.ReadFile(caCertPath)

Check warning on line 87 in internal/certs.go

View check run for this annotation

Codecov / codecov/patch

internal/certs.go#L87

Added line #L87 was not covered by tests
if err != nil {
return nil, fmt.Errorf("cannot read CA cert: %v", err)
}

caKeyBytes, err := ioutil.ReadFile(caKeyPath)
caKeyBytes, err := os.ReadFile(caKeyPath)

Check warning on line 92 in internal/certs.go

View check run for this annotation

Codecov / codecov/patch

internal/certs.go#L92

Added line #L92 was not covered by tests
if err != nil {
return nil, fmt.Errorf("cannot read CA key: %v", err)
}
Expand Down
5 changes: 2 additions & 3 deletions internal/retryafter_test.go
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
package internal

import (
"github.com/stretchr/testify/assert"
"math/rand"
"net/http"
"strconv"
"testing"
"time"

"github.com/stretchr/testify/assert"
)

func response503() *http.Response {
Expand All @@ -32,7 +33,6 @@ func assertDuration(t *testing.T, duration OptionalDuration, expected time.Durat

func TestExtractRetryAfterHeaderDelaySeconds(t *testing.T) {
// Generate random n > 0 int
rand.Seed(time.Now().UnixNano())
tigrannajaryan marked this conversation as resolved.
Show resolved Hide resolved
retryIntervalSec := rand.Intn(9999)

// Generate a 503 status code response with Retry-After = n header
Expand All @@ -58,7 +58,6 @@ func TestExtractRetryAfterHeaderDelaySeconds(t *testing.T) {
func TestExtractRetryAfterHeaderHttpDate(t *testing.T) {
// Generate a random n > 0 second duration
now := time.Now()
rand.Seed(now.UnixNano())
retryIntervalSec := rand.Intn(9999)
expectedDuration := time.Second * time.Duration(retryIntervalSec)

Expand Down
4 changes: 2 additions & 2 deletions internal/wsmessage.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ func DecodeWSMessage(bytes []byte, msg proto.Message) error {
}
// Skip the header. It really is just a single zero byte for now.
bytes = bytes[n:]
} else {
} /*else {
// Old message format. No header present.
}
}*/
echlebek marked this conversation as resolved.
Show resolved Hide resolved

// Decode WebSocket message as a Protobuf message.
err := proto.Unmarshal(bytes, msg)
Expand Down
2 changes: 1 addition & 1 deletion protobufshelpers/anyvaluehelpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func IsEqualAnyValue(v1, v2 *protobufs.AnyValue) bool {

case *protobufs.AnyValue_BytesValue:
v2, ok := v2.Value.(*protobufs.AnyValue_BytesValue)
return ok && bytes.Compare(v1.BytesValue, v2.BytesValue) == 0
return ok && bytes.Equal(v1.BytesValue, v2.BytesValue)

case *protobufs.AnyValue_ArrayValue:
v2, ok := v2.Value.(*protobufs.AnyValue_ArrayValue)
Expand Down
2 changes: 2 additions & 0 deletions server/serverimpl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,7 @@ func TestServerHonoursClientRequestContentEncoding(t *testing.T) {
require.NoError(t, err)

req, err := http.NewRequest("POST", "http://"+settings.ListenEndpoint+settings.ListenPath, bytes.NewReader(b))
require.NoError(t, err)
req.Header.Set(headerContentType, contentTypeProtobuf)
req.Header.Set(headerContentEncoding, contentEncodingGzip)
resp, err := hc.Do(req)
Expand Down Expand Up @@ -700,6 +701,7 @@ func TestServerHonoursAcceptEncoding(t *testing.T) {
b, err := proto.Marshal(&sendMsg)
require.NoError(t, err)
req, err := http.NewRequest("POST", "http://"+settings.ListenEndpoint+settings.ListenPath, bytes.NewReader(b))
require.NoError(t, err)
req.Header.Set(headerContentType, contentTypeProtobuf)
req.Header.Set(headerAcceptEncoding, contentEncodingGzip)
resp, err := hc.Do(req)
Expand Down
Loading