Issue crc-org#5037 Address review feedback for bundle download command

Author: sebrandon1

cmd/crc/cmd/bundle/download.go

@@ -50,9 +50,9 @@ func getDownloadCmd(config *crcConfig.Config) *cobra.Command {
 }
 
 func runDownload(args []string, preset crcPreset.Preset, force bool) error {
-	// Disk space check (simple check for ~10GB free)
-	// This is a basic check, more robust checking would require syscall/windows specific implementations
-	// We skip this for now to avoid adding heavy OS-specific deps, assuming user manages disk space or download fails naturally.
+	if len(args) > 2 {
+		return fmt.Errorf("too many arguments: expected at most 2 (version, architecture), got %d", len(args))
+	}
 
 	// If no args, use default bundle path
 	if len(args) == 0 {
@@ -77,7 +77,7 @@ func runDownload(args []string, preset crcPreset.Preset, force bool) error {
 	// Check if version is partial (Major.Minor) and resolve it if necessary
 	resolvedVersion, err := resolveOpenShiftVersion(preset, version)
 	if err != nil {
-		logging.Warnf("Could not resolve version %s: %v. Trying with original version string.", version, err)
+		return fmt.Errorf("failed to resolve version %s: %w", version, err)
 	} else if resolvedVersion != version {
 		logging.Debugf("Resolved version %s to %s", version, resolvedVersion)
 		version = resolvedVersion
@@ -147,8 +147,10 @@ func getVerifiedHashForCustomVersion(sigURL string, bundleName string) (string,
 
 	lines := strings.Split(verifiedHashes, "\n")
 	for _, line := range lines {
+		line = strings.TrimSpace(line)
 		if strings.HasSuffix(line, bundleName) {
 			sha256sum := strings.TrimSuffix(line, "  "+bundleName)
+			sha256sum = strings.TrimSpace(sha256sum)
 			return sha256sum, nil
 		}
 	}

cmd/crc/cmd/bundle/prune.go

@@ -1,9 +1,12 @@
 package bundle
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
+	"regexp"
 	"sort"
+	"strconv"
 	"strings"
 
 	"github.com/crc-org/crc/v2/pkg/crc/constants"
@@ -23,6 +26,45 @@ func getPruneCmd() *cobra.Command {
 	}
 }
 
+type bundleVersionInfo struct {
+	name  string
+	major int
+	minor int
+	patch int
+	arch  string
+}
+
+// parseBundleVersion parses a bundle filename like "crc_vfkit_4.19.13_arm64.crcbundle"
+// and extracts the version parts and architecture.
+func parseBundleVersion(filename string) (bundleVersionInfo, error) {
+	re := regexp.MustCompile(`^crc(?:_okd|_microshift)?_(?:vfkit|libvirt|hyperv)_(\d+)\.(\d+)\.(\d+)_([a-z0-9]+)\.crcbundle$`)
+	matches := re.FindStringSubmatch(filename)
+	if matches == nil {
+		return bundleVersionInfo{}, fmt.Errorf("filename %q does not match expected bundle pattern", filename)
+	}
+
+	major, err := strconv.Atoi(matches[1])
+	if err != nil {
+		return bundleVersionInfo{}, fmt.Errorf("invalid major version in %q: %w", filename, err)
+	}
+	minor, err := strconv.Atoi(matches[2])
+	if err != nil {
+		return bundleVersionInfo{}, fmt.Errorf("invalid minor version in %q: %w", filename, err)
+	}
+	patch, err := strconv.Atoi(matches[3])
+	if err != nil {
+		return bundleVersionInfo{}, fmt.Errorf("invalid patch version in %q: %w", filename, err)
+	}
+
+	return bundleVersionInfo{
+		name:  filename,
+		major: major,
+		minor: minor,
+		patch: patch,
+		arch:  matches[4],
+	}, nil
+}
+
 func runPrune(keep int) error {
 	cacheDir := constants.MachineCacheDir
 	if _, err := os.Stat(cacheDir); os.IsNotExist(err) {
@@ -35,37 +77,51 @@ func runPrune(keep int) error {
 		return err
 	}
 
-	var bundleFiles []os.DirEntry
-	for _, file := range files {
-		if strings.HasSuffix(file.Name(), ".crcbundle") {
-			bundleFiles = append(bundleFiles, file)
-		}
+	// Group bundles by major.minor + arch
+	type groupKey struct {
+		major int
+		minor int
+		arch  string
 	}
+	groups := make(map[groupKey][]bundleVersionInfo)
 
-	if len(bundleFiles) <= keep {
-		logging.Infof("Nothing to prune (found %d bundles, keeping %d)", len(bundleFiles), keep)
-		return nil
+	for _, file := range files {
+		if !strings.HasSuffix(file.Name(), ".crcbundle") {
+			continue
+		}
+		info, err := parseBundleVersion(file.Name())
+		if err != nil {
+			logging.Debugf("Skipping unrecognized bundle file: %s", file.Name())
+			continue
+		}
+		key := groupKey{major: info.major, minor: info.minor, arch: info.arch}
+		groups[key] = append(groups[key], info)
 	}
 
-	// Sort by modification time, newest first
-	sort.Slice(bundleFiles, func(i, j int) bool {
-		infoI, errI := bundleFiles[i].Info()
-		infoJ, errJ := bundleFiles[j].Info()
-		if errI != nil || errJ != nil {
-			// If we can't get info, treat as oldest (sort to end for pruning)
-			return errJ != nil && errI == nil
+	pruned := false
+	for _, bundles := range groups {
+		if len(bundles) <= keep {
+			continue
 		}
-		return infoI.ModTime().After(infoJ.ModTime())
-	})
-
-	for i := keep; i < len(bundleFiles); i++ {
-		file := bundleFiles[i]
-		filePath := filepath.Join(cacheDir, file.Name())
-		logging.Infof("Pruning old bundle: %s", file.Name())
-		if err := os.RemoveAll(filePath); err != nil {
-			logging.Errorf("Failed to remove %s: %v", filePath, err)
+
+		// Sort by patch version descending (newest first)
+		sort.Slice(bundles, func(i, j int) bool {
+			return bundles[i].patch > bundles[j].patch
+		})
+
+		for i := keep; i < len(bundles); i++ {
+			filePath := filepath.Join(cacheDir, bundles[i].name)
+			logging.Infof("Pruning old bundle: %s", bundles[i].name)
+			if err := os.RemoveAll(filePath); err != nil {
+				logging.Errorf("Failed to remove %s: %v", filePath, err)
+			}
+			pruned = true
 		}
 	}
 
+	if !pruned {
+		logging.Infof("Nothing to prune")
+	}
+
 	return nil
 }