From f8a97c7349c053cef4a58ca7eac0144719fd516a Mon Sep 17 00:00:00 2001 From: Or Shachar Date: Sat, 25 Nov 2023 13:07:04 +0200 Subject: [PATCH] chore: Fixed warnings, added small UT, added CI build (#8) * Fixed warnings, added small UT, added CI build * fixed golint errors Signed-off-by: or-shachar --------- Signed-off-by: or-shachar --- .github/workflows/go.yml | 36 ++++++++++++++ cacheproc/cacheproc.go | 11 ++++- cachers/combined.go | 2 +- cachers/counts.go | 2 +- cachers/disk.go | 10 ++-- cachers/http.go | 2 +- cachers/s3.go | 2 +- cachers/timekeeper.go | 2 +- cmd/go-cacher-server/cacher-server.go | 4 +- cmd/go-cacher/cacher.go | 63 ++++++++++++++++--------- cmd/go-cacher/cacher_test.go | 68 +++++++++++++++++++++++++++ go.mod | 4 ++ go.sum | 9 ++++ 13 files changed, 178 insertions(+), 37 deletions(-) create mode 100644 .github/workflows/go.yml create mode 100644 cmd/go-cacher/cacher_test.go diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml new file mode 100644 index 0000000..4e71e0c --- /dev/null +++ b/.github/workflows/go.yml @@ -0,0 +1,36 @@ +# This workflow will build a golang project +# For more information see: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-go + +name: Go + +on: + push: + branches: [ "main" ] + pull_request: + branches: [ "main" ] + +jobs: + + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + + - name: Set up Go + uses: actions/setup-go@v4 + with: + go-version: stable + check-latest: true + + - name: Build + run: go build -v ./... + + - name: Test + run: go test -v ./... + + - name: golangci-lint + uses: golangci/golangci-lint-action@v3 + with: + version: latest + args: --timeout 5m + install-mode: "binary" \ No newline at end of file diff --git a/cacheproc/cacheproc.go b/cacheproc/cacheproc.go index 95a8f61..6c90735 100644 --- a/cacheproc/cacheproc.go +++ b/cacheproc/cacheproc.go @@ -25,9 +25,12 @@ import ( "github.com/bradfitz/go-tool-cache/wire" ) +type cacherCtxKey string + var ( ErrUnknownCommand = errors.New("unknown command") ErrNoOutputID = errors.New("no outputID") + requestIDKey = cacherCtxKey("requestID") ) // Process implements the cmd/go JSON protocol over stdin & stdout via three @@ -89,7 +92,7 @@ func (p *Process) Run(ctx context.Context) error { } wg.Go(func() error { res := &wire.Response{ID: req.ID} - ctx := context.WithValue(ctx, "requestID", &req) + ctx := context.WithValue(ctx, requestIDKey, &req) if err := p.handleRequest(ctx, &req, res); err != nil { res.Err = err.Error() } @@ -99,8 +102,12 @@ func (p *Process) Run(ctx context.Context) error { _ = bw.Flush() return nil }) + select { + case <-ctx.Done(): + return wg.Wait() + default: + } } - return wg.Wait() } func (p *Process) handleRequest(ctx context.Context, req *wire.Request, res *wire.Response) error { diff --git a/cachers/combined.go b/cachers/combined.go index 8e04c27..786aa08 100644 --- a/cachers/combined.go +++ b/cachers/combined.go @@ -108,7 +108,7 @@ func (l *CombinedCache) Put(ctx context.Context, actionID, outputID string, size e := l.remoteCache.Put(ctx, actionID, outputID, size, putBody) return "", e }) - pw.Close() + _ = pw.Close() if err := wg.Wait(); err != nil { log.Printf("[%s]\terror: %v", l.localCache.Kind(), err) return "", err diff --git a/cachers/counts.go b/cachers/counts.go index 29d4808..b764ee4 100644 --- a/cachers/counts.go +++ b/cachers/counts.go @@ -29,7 +29,7 @@ type LocalCacheWithCounts struct { } func (l *LocalCacheWithCounts) Kind() string { - return l.Kind() + return l.cache.Kind() } type RemoteCacheWithCounts struct { diff --git a/cachers/disk.go b/cachers/disk.go index c81665f..c9dc082 100644 --- a/cachers/disk.go +++ b/cachers/disk.go @@ -40,12 +40,12 @@ func NewSimpleDiskCache(verbose bool, dir string) *SimpleDiskCache { var _ LocalCache = &SimpleDiskCache{} -func (dc *SimpleDiskCache) Start(ctx context.Context) error { +func (dc *SimpleDiskCache) Start(context.Context) error { log.Printf("[%s]\tlocal cache in %s", dc.Kind(), dc.dir) - return nil + return os.MkdirAll(dc.dir, 0755) } -func (dc *SimpleDiskCache) Get(ctx context.Context, actionID string) (outputID, diskPath string, err error) { +func (dc *SimpleDiskCache) Get(_ context.Context, actionID string) (outputID, diskPath string, err error) { actionFile := filepath.Join(dc.dir, fmt.Sprintf("a-%s", actionID)) ij, err := os.ReadFile(actionFile) if err != nil { @@ -66,7 +66,7 @@ func (dc *SimpleDiskCache) Get(ctx context.Context, actionID string) (outputID, return ie.OutputID, filepath.Join(dc.dir, fmt.Sprintf("o-%v", ie.OutputID)), nil } -func (dc *SimpleDiskCache) Put(ctx context.Context, actionID, objectID string, size int64, body io.Reader) (diskPath string, _ error) { +func (dc *SimpleDiskCache) Put(_ context.Context, actionID, objectID string, size int64, body io.Reader) (diskPath string, _ error) { file := filepath.Join(dc.dir, fmt.Sprintf("o-%s", objectID)) // Special case empty files; they're both common and easier to do race-free. @@ -75,7 +75,7 @@ func (dc *SimpleDiskCache) Put(ctx context.Context, actionID, objectID string, s if err != nil { return "", err } - zf.Close() + _ = zf.Close() } else { wrote, err := writeAtomic(file, body) if err != nil { diff --git a/cachers/http.go b/cachers/http.go index 31b1420..4c699fe 100644 --- a/cachers/http.go +++ b/cachers/http.go @@ -36,7 +36,7 @@ func NewHttpCache(baseURL string, verbose bool) *HTTPCache { } } -func (c *HTTPCache) Start(ctx context.Context) error { +func (c *HTTPCache) Start(context.Context) error { log.Printf("[%s]\tconfigured to %s", c.Kind(), c.baseURL) return nil } diff --git a/cachers/s3.go b/cachers/s3.go index d8090bc..a4fc468 100644 --- a/cachers/s3.go +++ b/cachers/s3.go @@ -40,7 +40,7 @@ func (s *S3Cache) Kind() string { return "s3" } -func (s *S3Cache) Start(ctx context.Context) error { +func (s *S3Cache) Start(context.Context) error { log.Printf("[%s]\tconfigured to s3://%s/%s", s.Kind(), s.bucket, s.prefix) return nil } diff --git a/cachers/timekeeper.go b/cachers/timekeeper.go index 1b176b9..27ac2c4 100644 --- a/cachers/timekeeper.go +++ b/cachers/timekeeper.go @@ -73,7 +73,7 @@ func (c *timeKeeper) DoWithMeasure(bytesCount int64, f func() (string, error)) ( // formatBytes formats a number of bytes into a human-readable string. func formatBytes(size float64) string { const ( - b = 1 << (10 * iota) + _ = 1 << (10 * iota) kb mb gb diff --git a/cmd/go-cacher-server/cacher-server.go b/cmd/go-cacher-server/cacher-server.go index 197aac9..7c70c46 100644 --- a/cmd/go-cacher-server/cacher-server.go +++ b/cmd/go-cacher-server/cacher-server.go @@ -95,7 +95,7 @@ func (s *server) ServeHTTP(w http.ResponseWriter, r *http.Request) { case strings.HasPrefix(r.URL.Path, "/output/"): s.handleGetOutput(w, r) case r.URL.Path == "/": - io.WriteString(w, "hi") + _, _ = io.WriteString(w, "hi") default: http.Error(w, "not found", http.StatusNotFound) } @@ -150,7 +150,7 @@ func (s *server) handleGetAction(w http.ResponseWriter, r *http.Request) { return } w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(&cachers.ActionValue{ + _ = json.NewEncoder(w).Encode(&cachers.ActionValue{ OutputID: outputID, Size: fi.Size(), }) diff --git a/cmd/go-cacher/cacher.go b/cmd/go-cacher/cacher.go index 6cd5bc6..88b47a7 100644 --- a/cmd/go-cacher/cacher.go +++ b/cmd/go-cacher/cacher.go @@ -46,14 +46,24 @@ var ( verbose = flag.Bool("verbose", false, "be verbose") ) -func getAwsConfigFromEnv(ctx context.Context) (*aws.Config, error) { +type Env interface { + Get(key string) string +} + +type osEnv struct{} + +func (osEnv) Get(key string) string { + return os.Getenv(key) +} + +func getAwsConfigFromEnv(ctx context.Context, env Env) (*aws.Config, error) { // read from env - awsRegion := os.Getenv(envVarS3CacheRegion) + awsRegion := env.Get(envVarS3CacheRegion) if awsRegion == "" { return nil, nil } - accessKey := os.Getenv(envVarS3AwsAccessKey) - secretAccessKey := os.Getenv(envVarS3AwsSecretAccessKey) + accessKey := env.Get(envVarS3AwsAccessKey) + secretAccessKey := env.Get(envVarS3AwsSecretAccessKey) if accessKey != "" && secretAccessKey != "" { cfg, err := config.LoadDefaultConfig(ctx, config.WithRegion(awsRegion), @@ -68,9 +78,9 @@ func getAwsConfigFromEnv(ctx context.Context) (*aws.Config, error) { } return &cfg, nil } - credsProfile := os.Getenv(envVarS3AwsCredsProfile) + credsProfile := env.Get(envVarS3AwsCredsProfile) if credsProfile != "" { - cfg, err := config.LoadDefaultConfig(context.TODO(), config.WithRegion(awsRegion), config.WithSharedConfigProfile(credsProfile)) + cfg, err := config.LoadDefaultConfig(ctx, config.WithRegion(awsRegion), config.WithSharedConfigProfile(credsProfile)) if err != nil { return nil, err } @@ -79,17 +89,17 @@ func getAwsConfigFromEnv(ctx context.Context) (*aws.Config, error) { return nil, nil } -func maybeS3Cache(ctx context.Context) (cachers.RemoteCache, error) { - awsConfig, err := getAwsConfigFromEnv(ctx) +func maybeS3Cache(ctx context.Context, env Env) (cachers.RemoteCache, error) { + awsConfig, err := getAwsConfigFromEnv(ctx, env) if err != nil { return nil, err } - bucket := os.Getenv(envVarS3BucketName) + bucket := env.Get(envVarS3BucketName) if bucket == "" || awsConfig == nil { // We need at least name of bucket and valid aws config return nil, nil } - cacheKey := os.Getenv(envVarS3CacheKey) + cacheKey := env.Get(envVarS3CacheKey) if cacheKey == "" { cacheKey = defaultCacheKey } @@ -98,17 +108,21 @@ func maybeS3Cache(ctx context.Context) (cachers.RemoteCache, error) { return s3Cache, nil } -func getCache(ctx context.Context, local cachers.LocalCache, verbose bool) cachers.LocalCache { - remote, err := maybeS3Cache(ctx) +func getCache(ctx context.Context, env Env, verbose bool) cachers.LocalCache { + dir := getDir(env) + var local cachers.LocalCache = cachers.NewSimpleDiskCache(verbose, dir) + + remote, err := maybeS3Cache(ctx, env) if err != nil { log.Fatal(err) } if remote == nil { - remote, err = maybeHttpCache() + remote, err = maybeHttpCache(env) if err != nil { log.Fatal(err) } } + if remote != nil { return cachers.NewCombinedCache(local, remote, verbose) } @@ -118,17 +132,16 @@ func getCache(ctx context.Context, local cachers.LocalCache, verbose bool) cache return local } -func maybeHttpCache() (cachers.RemoteCache, error) { - serverBase := os.Getenv(envVarHttpCacheServerBase) +func maybeHttpCache(env Env) (cachers.RemoteCache, error) { + serverBase := env.Get(envVarHttpCacheServerBase) if serverBase == "" { return nil, nil } return cachers.NewHttpCache(serverBase, *verbose), nil } -func main() { - flag.Parse() - dir := os.Getenv(envVarDiskCacheDir) +func getDir(env Env) string { + dir := env.Get(envVarDiskCacheDir) if dir == "" { d, err := os.UserCacheDir() if err != nil { @@ -137,14 +150,18 @@ func main() { d = filepath.Join(d, "go-cacher") dir = d } - if err := os.MkdirAll(dir, 0755); err != nil { - log.Fatal(err) - } + return dir +} + +func main() { + flag.Parse() + env := &osEnv{} - var localCache cachers.LocalCache = cachers.NewSimpleDiskCache(*verbose, dir) ctx, cancel := context.WithCancel(context.Background()) defer cancel() - proc := cacheproc.NewCacheProc(getCache(ctx, localCache, *verbose)) + + cache := getCache(ctx, env, *verbose) + proc := cacheproc.NewCacheProc(cache) if err := proc.Run(ctx); err != nil { log.Fatal(err) } diff --git a/cmd/go-cacher/cacher_test.go b/cmd/go-cacher/cacher_test.go new file mode 100644 index 0000000..fa1af27 --- /dev/null +++ b/cmd/go-cacher/cacher_test.go @@ -0,0 +1,68 @@ +package main + +import ( + "context" + "maps" + "testing" + + "github.com/stretchr/testify/assert" +) + +type mapEnv struct { + m map[string]string +} + +var _ Env = &mapEnv{} + +func (m *mapEnv) Get(key string) string { + return m.m[key] +} + +func TestMaybeS3Cache(t *testing.T) { + fullMap := map[string]string{ + envVarS3BucketName: "bucket", + envVarS3CacheRegion: "region", + envVarS3AwsAccessKey: "accessKey", + envVarS3AwsSecretAccessKey: "secretAccessKey", + } + for k := range fullMap { + missingMap := map[string]string{} + maps.Copy(missingMap, fullMap) + delete(missingMap, k) + t.Run("should return nil if "+k+" is missing", func(t *testing.T) { + env := &mapEnv{m: missingMap} + client, err := maybeS3Cache(context.TODO(), env) + assert.NoError(t, err) + assert.Nil(t, client) + }) + } + + t.Run("should return cache if all expected env vars exists", func(t *testing.T) { + env := &mapEnv{ + m: fullMap, + } + client, err := maybeS3Cache(context.TODO(), env) + assert.NoError(t, err) + assert.NotNil(t, client) + }) +} + +func TestMaybeHttpCache(t *testing.T) { + t.Run("should return nil if "+envVarHttpCacheServerBase+" is missing", func(t *testing.T) { + env := &mapEnv{m: map[string]string{}} + client, err := maybeHttpCache(env) + assert.NoError(t, err) + assert.Nil(t, client) + }) + + t.Run("should return cache if "+envVarHttpCacheServerBase+" env vars exists", func(t *testing.T) { + env := &mapEnv{ + m: map[string]string{ + envVarHttpCacheServerBase: "http://localhost:8080", + }, + } + client, err := maybeHttpCache(env) + assert.NoError(t, err) + assert.NotNil(t, client) + }) +} diff --git a/go.mod b/go.mod index 57ae363..2635e38 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ require ( github.com/aws/aws-sdk-go-v2/credentials v1.15.2 github.com/aws/aws-sdk-go-v2/service/s3 v1.42.1 github.com/aws/smithy-go v1.16.0 + github.com/stretchr/testify v1.8.4 golang.org/x/sync v0.5.0 ) @@ -25,4 +26,7 @@ require ( github.com/aws/aws-sdk-go-v2/service/sso v1.17.1 // indirect github.com/aws/aws-sdk-go-v2/service/ssooidc v1.19.1 // indirect github.com/aws/aws-sdk-go-v2/service/sts v1.25.1 // indirect + github.com/davecgh/go-spew v1.1.1 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/go.sum b/go.sum index 80a2ef6..c6e52e4 100644 --- a/go.sum +++ b/go.sum @@ -34,8 +34,17 @@ github.com/aws/aws-sdk-go-v2/service/sts v1.25.1 h1:txgVXIXWPXyqdiVn92BV6a/rgtpX github.com/aws/aws-sdk-go-v2/service/sts v1.25.1/go.mod h1:VAiJiNaoP1L89STFlEMgmHX1bKixY+FaP+TpRFrmyZ4= github.com/aws/smithy-go v1.16.0 h1:gJZEH/Fqh+RsvlJ1Zt4tVAtV6bKkp3cC+R6FCZMNzik= github.com/aws/smithy-go v1.16.0/go.mod h1:NukqUGpCZIILqqiV0NIjeFh24kd/FAa4beRb6nbIUPE= +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/google/go-cmp v0.5.8 h1:e6P7q2lk1O+qJJb4BtCQXlK8vWEO8V1ZeuEdJNOqZyg= github.com/google/go-cmp v0.5.8/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= github.com/jmespath/go-jmespath v0.4.0/go.mod h1:T8mJZnbsbmF+m6zOOFylbeCJqk5+pHWvzYPziyZiYoo= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk= +github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo= golang.org/x/sync v0.5.0 h1:60k92dhOjHxJkrqnwsfl8KuaHbn/5dl0lUPUklKo3qE= golang.org/x/sync v0.5.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=