Refactor code and tests

- Rename flag and internal arguments to clarify that this caching is specific to artifactory
- Refactor tests to cover both the happy path and critical error paths
- Move code and test to resource*.go
- Add integrity checking for cached artifacts

cmd/add.go

@@ -21,7 +21,7 @@ func addAdd(cmd *cobra.Command) {
 	addCmd.Flags().String("algo", internal.RecommendedAlgo, "Integrity algorithm")
 	addCmd.Flags().String("filename", "", "Target file name to use when downloading the resource")
 	addCmd.Flags().StringArray("tag", []string{}, "Resource tags")
-	addCmd.Flags().String("cache", "", "Artifactory cache URL")
+	addCmd.Flags().String("artifactory-cache-url", "", "Artifactory cache URL")
 	cmd.AddCommand(addCmd)
 }
 
@@ -31,15 +31,14 @@ func runAdd(cmd *cobra.Command, args []string) error {
 		return err
 	}
 	// Get cache URL
-	cacheURL, err := cmd.Flags().GetString("cache")
+	ArtifactoryCacheURL, err := cmd.Flags().GetString("artifactory-cache-url")
 	if err != nil {
 		return err
 	}
-	// Check token if cache is requested
-	if cacheURL != "" {
-		token := os.Getenv("GRABIT_ARTIFACTORY_TOKEN")
+	if ArtifactoryCacheURL != "" {
+		token := os.Getenv(internal.GRABIT_ARTIFACTORY_TOKEN_ENV_VAR)
 		if token == "" {
-			return fmt.Errorf("GRABIT_ARTIFACTORY_TOKEN environment variable is not set")
+			return fmt.Errorf("%s environment variable is not set", internal.GRABIT_ARTIFACTORY_TOKEN_ENV_VAR)
 		}
 	}
 	lock, err := internal.NewLock(lockFile, true)
@@ -58,7 +57,7 @@ func runAdd(cmd *cobra.Command, args []string) error {
 	if err != nil {
 		return err
 	}
-	err = lock.AddResource(args, algo, tags, filename, cacheURL)
+	err = lock.AddResource(args, algo, tags, filename, ArtifactoryCacheURL)
 	if err != nil {
 		return err
 	}

cmd/add_test.go

@@ -2,49 +2,27 @@ package cmd
 
 import (
 	"fmt"
-	"net/http"
 	"testing"
 
+	"github.com/cisco-open/grabit/internal"
 	"github.com/cisco-open/grabit/test"
 	"github.com/stretchr/testify/assert"
 )
 
 func TestRunAdd(t *testing.T) {
-	// Set the GRABIT_ARTIFACTORY_TOKEN environment variable.
-	t.Setenv("GRABIT_ARTIFACTORY_TOKEN", "test-token")
-
-	// Setup HTTP handler for the resource
-	handler := func(w http.ResponseWriter, r *http.Request) {
-		_, err := w.Write([]byte(`abcdef`))
-		if err != nil {
-			t.Fatal(err)
-		}
-	}
-	port, server := test.HttpHandler(handler)
-	defer server.Close()
-
-	// Setup dummy cache server
-	cacheHandler := func(w http.ResponseWriter, r *http.Request) {
-		if r.Method == "PUT" {
-			w.WriteHeader(http.StatusCreated)
-		}
-	}
-	cachePort, cacheServer := test.HttpHandler(cacheHandler)
-	defer cacheServer.Close()
-
-	// Create empty lockfile
-	lockFile := test.TmpFile(t, "")
-
+	port := test.TestHttpHandler("abcdef", t)
 	cmd := NewRootCmd()
-	// Add cache URL to the command
-	cacheURL := fmt.Sprintf("http://localhost:%d", cachePort)
-	cmd.SetArgs([]string{
-		"-f", lockFile,
-		"add",
-		fmt.Sprintf("http://localhost:%d/test.html", port),
-		"--cache", cacheURL,
-	})
+	cmd.SetArgs([]string{"-f", test.TmpFile(t, ""), "add", fmt.Sprintf("http://localhost:%d/test.html", port)})
+	err := cmd.Execute()
+	assert.Nil(t, err)
+}
 
+func TestRunAddWithArtifactoryCache(t *testing.T) {
+	t.Setenv(internal.GRABIT_ARTIFACTORY_TOKEN_ENV_VAR, "artifactory-token")
+	port := test.TestHttpHandler("abcdef", t)
+	artPort := test.TestHttpHandler("abcdef", t)
+	cmd := NewRootCmd()
+	cmd.SetArgs([]string{"-f", test.TmpFile(t, ""), "add", fmt.Sprintf("http://localhost:%d/test.html", port), "--artifactory-cache-url", fmt.Sprintf("http://localhost:%d/artifactory", artPort)})
 	err := cmd.Execute()
 	assert.Nil(t, err)
 }

cmd/delete.go

@@ -28,7 +28,10 @@ func runDel(cmd *cobra.Command, args []string) error {
 		return err
 	}
 	for _, r := range args {
-		lock.DeleteResource(r)
+		err = lock.DeleteResource(r)
+		if err != nil {
+			return err
+		}
 	}
 	err = lock.Save()
 	if err != nil {

internal/lock.go

@@ -8,26 +8,14 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"net/http"
 	"os"
-	"path"
-	"path/filepath"
 	"strconv"
-	"strings"
 
-	"github.com/carlmjohnson/requests"
 	toml "github.com/pelletier/go-toml/v2"
-	"github.com/rs/zerolog/log"
 )
 
 var COMMENT_PREFIX = "//"
 
-// getArtifactoryURL constructs the URL for an artifact in Artifactory
-func getArtifactoryURL(baseURL, integrity string) string {
-	return fmt.Sprintf("%s/%s", baseURL, integrity)
-
-}
-
 // Lock represents a grabit lockfile.
 type Lock struct {
 	path string
@@ -68,90 +56,29 @@ func (l *Lock) AddResource(paths []string, algo string, tags []string, filename
 		}
 	}
 	r, err := NewResourceFromUrl(paths, algo, tags, filename, cacheURL)
+
 	if err != nil {
 		return err
 	}
 
-	// If cache URL is provided, handles Artifactory upload
-	if cacheURL != "" {
-		token := os.Getenv("GRABIT_ARTIFACTORY_TOKEN")
-		if token == "" {
-			return fmt.Errorf("GRABIT_ARTIFACTORY_TOKEN environment variable is not set")
-		}
-
-		// Add context here
-		ctx := context.Background()
-		path, err := GetUrltoTempFile(paths[0], token, ctx)
-		if err != nil {
-			return fmt.Errorf("failed to get file for cache: %s", err)
-		}
-		defer os.Remove(path)
-
-		// Upload to Artifactory using hash as filename
-		err = uploadToArtifactory(path, cacheURL, r.Integrity)
-		if err != nil {
-			return fmt.Errorf("failed to upload to cache: %v", err)
-		}
-	}
-
 	l.conf.Resource = append(l.conf.Resource, *r)
 	return nil
 }
 
-func uploadToArtifactory(filePath, cacheURL, integrity string) error {
-	token := os.Getenv("GRABIT_ARTIFACTORY_TOKEN")
-	if token == "" {
-		return fmt.Errorf("GRABIT_ARTIFACTORY_TOKEN environment variable is not set")
-	}
-
-	// Use the integrity value directly for the URL
-	artifactoryURL := getArtifactoryURL(cacheURL, integrity)
-
-	// Upload the file using the requests package
-	err := requests.
-		URL(artifactoryURL).
-		Method(http.MethodPut).
-		Header("Authorization", fmt.Sprintf("Bearer %s", token)).
-		BodyFile(filePath). // Using BodyFile instead of ReadFile
-		Fetch(context.Background())
-
-	if err != nil {
-		return fmt.Errorf("upload failed: %w", err)
-	}
-
-	return nil
-}
-func (l *Lock) DeleteResource(path string) {
+func (l *Lock) DeleteResource(path string) error {
 	newStatements := []Resource{}
 	for _, r := range l.conf.Resource {
 		if !r.Contains(path) {
 			newStatements = append(newStatements, r)
-		} else if r.Contains(path) && r.CacheUri != "" {
-			token := os.Getenv("GRABIT_ARTIFACTORY_TOKEN")
-			if token == "" {
-				log.Warn().Msg("Warning: Unable to delete from Artifactory: GRABIT_ARTIFACTORY_TOKEN not set.")
-
-				continue
-			}
-
-			artifactoryURL := getArtifactoryURL(r.CacheUri, r.Integrity)
-
-			err := deleteCache(artifactoryURL, token)
+		} else {
+			err := r.Delete()
 			if err != nil {
-				log.Warn().Msg("Warning: Unable to delete from Artifactory")
+				return fmt.Errorf("Failed to delete resource '%s': %w", path, err)
 			}
 		}
 	}
 	l.conf.Resource = newStatements
-}
-
-func deleteCache(url, token string) error {
-	// Create and send a DELETE request with an Authorization header.
-	return requests.
-		URL(url).
-		Method(http.MethodDelete).
-		Header("Authorization", fmt.Sprintf("Bearer %s", token)).
-		Fetch(context.Background())
+	return nil
 }
 
 const NoFileMode = os.FileMode(0)
@@ -246,47 +173,6 @@ func (l *Lock) Download(dir string, tags []string, notags []string, perm string,
 	for i, r := range filteredResources {
 		resource := r
 		go func() {
-			// Try Artifactory first if available
-			if resource.CacheUri != "" {
-				token := os.Getenv("GRABIT_ARTIFACTORY_TOKEN")
-				if token != "" {
-					artifactoryURL := getArtifactoryURL(resource.CacheUri, resource.Integrity)
-					filename := resource.Filename
-					if filename == "" {
-						filename = path.Base(resource.Urls[0])
-					}
-					fullPath := filepath.Join(dir, filename)
-
-					// Use getUrl with bearer token
-					tmpPath, err := getUrl(artifactoryURL, fullPath, token, ctx)
-					if err == nil {
-						//  integrity check
-						algo, err := getAlgoFromIntegrity(resource.Integrity)
-						if err != nil {
-							errorCh <- err
-							return
-						}
-						err = checkIntegrityFromFile(tmpPath, algo, resource.Integrity, artifactoryURL)
-						if err != nil {
-							errorCh <- err
-							return
-						}
-						if mode != NoFileMode {
-							err = os.Chmod(tmpPath, mode.Perm())
-						}
-						if err == nil {
-							errorCh <- nil
-							if statusLine != nil {
-								statusLine.Increment(i)
-							}
-							return
-						}
-					}
-					if strings.Contains(err.Error(), "lookup invalid") || strings.Contains(err.Error(), "dial tcp") {
-						fmt.Printf("Failed to download from Artifactory, falling back to original URL: %v\n", err)
-					}
-				}
-			}
 
 			err := resource.Download(dir, mode, ctx)
 			errorCh <- err

internal/lock_test.go

@@ -56,7 +56,8 @@ func TestLockManipulations(t *testing.T) {
 	assert.Equal(t, 2, len(lock.conf.Resource))
 	err = lock.Save()
 	assert.Nil(t, err)
-	lock.DeleteResource(resource)
+	err = lock.DeleteResource(resource)
+	assert.Nil(t, err)
 	assert.Equal(t, 1, len(lock.conf.Resource))
 }