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 <[email protected]>

---------

Signed-off-by: or-shachar <[email protected]>

.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"

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 {

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

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 {

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 {

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
 }

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
 }

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

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(),
 	})

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)
 	}

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)
+	})
+}

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
 )