Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @sebrandon1. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
WalkthroughAdds four bundle subcommands (download, list, clear, prune); implements remote version discovery, partial-version resolution, signature-verified downloads into the machine cache, cache inspection/cleanup (list/clear/prune), and updates bundle naming and mirror constants; test expectations for generated manpages updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as "crc CLI (bundle)"
participant Mirror as "Mirror (HTTP)"
participant Verifier as "GPG verifier"
participant FS as "Local cache (filesystem)"
User->>CLI: crc bundle download [version] [--force] [--preset] [--arch]
CLI->>CLI: parse flags (preset, arch, force)
alt partial version provided
CLI->>Mirror: fetch available versions (fetchAvailableVersions)
Mirror-->>CLI: HTML listing
CLI->>CLI: resolve to concrete version (resolveOpenShiftVersion)
end
CLI->>Mirror: fetch signature (.sig)
Mirror-->>CLI: signed payload
CLI->>Verifier: verify signature, extract SHA256
Verifier-->>CLI: verified hash
CLI->>Mirror: download bundle using expected hash
Mirror-->>CLI: bundle stream
CLI->>FS: write bundle to machine cache
FS-->>CLI: confirm write
CLI->>User: success / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cmd/crc/cmd/bundle/download.go (3)
86-96: Consider making the prune retention count configurable.The
keepvalue is hardcoded to 2. Consider exposing this as a flag (e.g.,--keep) to allow users to control how many bundles to retain.func getPruneCmd() *cobra.Command { - return &cobra.Command{ + cmd := &cobra.Command{ Use: "prune", Short: "Prune old CRC bundles", Long: "Keep only the most recent bundles and delete older ones to save space.", RunE: func(cmd *cobra.Command, args []string) error { - // Default keep 2 most recent - return runPrune(2) + keep, _ := cmd.Flags().GetInt("keep") + return runPrune(keep) }, } + cmd.Flags().IntP("keep", "k", 2, "Number of most recent bundles to keep") + return cmd }
253-261: Consider failing early when version resolution fails for partial versions.When a partial version (e.g.,
4.19) cannot be resolved, the code warns and continues with the original version string. This will likely result in a 404 error later since4.19isn't a valid bundle path. Consider failing early for better UX.resolvedVersion, err := resolveOpenShiftVersion(preset, version, verbose) if err != nil { - logging.Warnf("Could not resolve version %s: %v. Trying with original version string.", version, err) - } else if resolvedVersion != version { + // Only warn if it looks like a full version that just wasn't found + partialVersionRegex := regexp.MustCompile(`^(\d+\.\d+)$`) + if partialVersionRegex.MatchString(version) { + return fmt.Errorf("could not resolve version %s: %w", version, err) + } + logging.Warnf("Could not resolve version %s: %v. Trying with original version string.", version, err) + } + if resolvedVersion != "" && resolvedVersion != version { if verbose { logging.Infof("Resolved version %s to %s", version, resolvedVersion) } version = resolvedVersion }
370-378: Consider adding context support for request cancellation.The HTTP request doesn't use a context, which means long-running requests can't be cancelled by the user (e.g., via Ctrl+C). Consider accepting a context parameter.
-func fetchAvailableVersions(preset crcPreset.Preset) ([]*semver.Version, error) { +func fetchAvailableVersions(ctx context.Context, preset crcPreset.Preset) ([]*semver.Version, error) { baseURL := fmt.Sprintf("https://mirror.openshift.com/pub/openshift-v4/clients/crc/bundles/%s/", preset.String()) client := &http.Client{ Transport: httpproxy.HTTPTransport(), Timeout: 10 * time.Second, } - req, err := http.NewRequest("GET", baseURL, nil) + req, err := http.NewRequestWithContext(ctx, "GET", baseURL, nil) if err != nil { return nil, err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Makefile(1 hunks)cmd/crc/cmd/bundle/bundle.go(1 hunks)cmd/crc/cmd/bundle/download.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: verify-devcontainer
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
🔇 Additional comments (7)
Makefile (1)
94-97: LGTM!The new
buildtarget correctly mirrors theinstalltarget but usesgo buildinstead ofgo install, allowing developers to build without installing. This is a useful addition for development workflows.cmd/crc/cmd/bundle/bundle.go (1)
18-18: LGTM!Clean integration of the new download subcommand, following the existing pattern for adding subcommands.
cmd/crc/cmd/bundle/download.go (5)
29-62: LGTM!Well-structured command definition with appropriate flags and subcommands. The preset parsing with fallback to config is handled correctly.
173-214: LGTM!The list command implementation is clean with appropriate version filtering and cache indication.
307-335: LGTM!Solid signature verification implementation that properly validates the GPG signature before extracting the hash.
337-359: LGTM!Bundle name construction correctly handles different presets and operating systems.
436-470: LGTM!Version resolution logic correctly handles full versions, partial versions, and edge cases.
cmd/crc/cmd/bundle/download.go
Outdated
| // Sort by modification time, newest first | ||
| sort.Slice(bundleFiles, func(i, j int) bool { | ||
| infoI, _ := bundleFiles[i].Info() | ||
| infoJ, _ := bundleFiles[j].Info() | ||
| return infoI.ModTime().After(infoJ.ModTime()) | ||
| }) |
There was a problem hiding this comment.
Potential nil pointer dereference when Info() fails.
If Info() returns an error (e.g., file deleted between ReadDir and Info call, or permission issue), the returned FileInfo is nil, causing a panic when accessing ModTime().
// Sort by modification time, newest first
sort.Slice(bundleFiles, func(i, j int) bool {
- infoI, _ := bundleFiles[i].Info()
- infoJ, _ := bundleFiles[j].Info()
+ 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
+ }
return infoI.ModTime().After(infoJ.ModTime())
})
sebrandon1
force-pushed
the
add_bundle_download_cmd
branch
from
2ee63db to
45985b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/crc/cmd/bundle/download.go (1)
154-159: Potential nil pointer dereference whenInfo()fails.If
Info()returns an error (e.g., file deleted betweenReadDirandInfocall), the returnedFileInfoisnil, causing a panic when accessingModTime().Apply this diff to handle the error case:
sort.Slice(bundleFiles, func(i, j int) bool { - infoI, _ := bundleFiles[i].Info() - infoJ, _ := bundleFiles[j].Info() + infoI, errI := bundleFiles[i].Info() + infoJ, errJ := bundleFiles[j].Info() + if errI != nil || errJ != nil { + return errJ != nil && errI == nil + } return infoI.ModTime().After(infoJ.ModTime()) })
🧹 Nitpick comments (3)
cmd/crc/cmd/bundle/download.go (3)
86-96: Consider making the keep count configurable.The hardcoded
keep=2value could be exposed as a flag for flexibility (e.g.,--keep=N).func getPruneCmd() *cobra.Command { - return &cobra.Command{ + cmd := &cobra.Command{ Use: "prune", Short: "Prune old CRC bundles", Long: "Keep only the most recent bundles and delete older ones to save space.", RunE: func(cmd *cobra.Command, args []string) error { - // Default keep 2 most recent - return runPrune(2) + keep, _ := cmd.Flags().GetInt("keep") + return runPrune(keep) }, } + cmd.Flags().IntP("keep", "k", 2, "Number of most recent bundles to keep") + return cmd }
115-117: Preferos.Removefor regular files.
os.RemoveAllis designed for recursive directory removal. For single files,os.Removeis more explicit and semantically correct.- if err := os.RemoveAll(filePath); err != nil { + if err := os.Remove(filePath); err != nil {
365-379: Consider propagating context for cancellation support.The HTTP request doesn't use context, preventing graceful cancellation (e.g., via Ctrl+C). For CLI tools this is usually acceptable, but adding context would improve the user experience.
-func fetchAvailableVersions(preset crcPreset.Preset) ([]*semver.Version, error) { +func fetchAvailableVersions(ctx context.Context, preset crcPreset.Preset) ([]*semver.Version, error) { baseURL := fmt.Sprintf("https://mirror.openshift.com/pub/openshift-v4/clients/crc/bundles/%s/", preset.String()) client := &http.Client{ Transport: httpproxy.HTTPTransport(), Timeout: 10 * time.Second, } - req, err := http.NewRequest("GET", baseURL, nil) + req, err := http.NewRequestWithContext(ctx, "GET", baseURL, nil)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Makefile(1 hunks)cmd/crc/cmd/bundle/bundle.go(1 hunks)cmd/crc/cmd/bundle/download.go(1 hunks)cmd/crc/cmd/root_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- cmd/crc/cmd/bundle/bundle.go
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/crc/cmd/bundle/download.go (4)
pkg/download/download.go (1)
InMemory(101-126)pkg/crc/gpg/gpg.go (1)
GetVerifiedClearsignedMsgV3(41-68)pkg/crc/constants/keys.go (1)
RedHatReleaseKey(22-49)pkg/crc/network/httpproxy/proxy.go (1)
HTTPTransport(257-264)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: verify-devcontainer
🔇 Additional comments (7)
cmd/crc/cmd/root_test.go (1)
26-29: LGTM!The test expectations correctly account for the four new manpage files generated by the bundle download subcommands.
cmd/crc/cmd/bundle/download.go (6)
29-62: LGTM!The command setup is well-structured with appropriate flags and subcommand wiring. The preset validation via
ParsePresetEensures only valid presets are accepted.
173-214: LGTM!The list implementation correctly handles both partial and full version filtering, with appropriate fallback parsing logic and cache status indication.
289-305: LGTM - Good security practice!The implementation correctly enforces signature verification before download and fails fast if verification fails, preventing download of potentially tampered bundles.
307-335: LGTM!The signature verification properly uses GPG verification with the Red Hat release key and correctly parses the sha256sum file format.
348-355: Cross-OS bundle downloads are not supported.
runtime.GOOSis used to determine the OS-specific bundle suffix (vfkit/libvirt/hyperv), meaning users cannot download bundles for a different OS than their current one. Thearchitectureparameter allows cross-architecture downloads but not cross-OS.If this limitation is intentional, consider documenting it in the command's long description. If cross-OS downloads should be supported, an
--osflag could be added similar to--preset.
436-470: LGTM!The version resolution logic correctly distinguishes between full versions (returned as-is), partial versions (resolved to latest patch), and other formats (passed through). Since
fetchAvailableVersionsreturns versions sorted newest-first, the first matching major.minor will be the latest patch.
|
To me managing this makes lot of sense |
|
@albfan Do you know if the CI is failing because of my changes or because of something else? It seems like a CI problem? |
|
@sebrandon1 It shouldn't be related to your code as they have been failing for other PRs too. The OKD pipeline, for example, fails because of an expired certificate. |
|
Thanks @redbeam! Let me know if there's anything you spot that needs changing. |
|
/ok-to-test |
sebrandon1
force-pushed
the
add_bundle_download_cmd
branch
from
45985b5 to
5f22c48
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/crc/cmd/bundle/download.go (1)
152-157: Potential nil pointer dereference whenInfo()fails.This issue was flagged in a previous review. If
Info()returns an error, the returnedFileInfoisnil, causing a panic when accessingModTime().// Sort by modification time, newest first sort.Slice(bundleFiles, func(i, j int) bool { - infoI, _ := bundleFiles[i].Info() - infoJ, _ := bundleFiles[j].Info() + 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 + } return infoI.ModTime().After(infoJ.ModTime()) })
🧹 Nitpick comments (4)
cmd/crc/cmd/bundle/download.go (4)
84-94: Consider making the keep count configurable.The hardcoded
keep=2value works, but users might want to retain more or fewer bundles depending on their use case.func getPruneCmd() *cobra.Command { - return &cobra.Command{ + cmd := &cobra.Command{ Use: "prune", Short: "Prune old CRC bundles", Long: "Keep only the most recent bundles and delete older ones to save space.", RunE: func(cmd *cobra.Command, args []string) error { - // Default keep 2 most recent - return runPrune(2) + keep, _ := cmd.Flags().GetInt("keep") + return runPrune(keep) }, } + cmd.Flags().IntP("keep", "k", 2, "Number of recent bundles to keep") + return cmd }
214-221: Cache check is not architecture-aware.This function uses
BundleForPresetwhich hardcodesruntime.GOARCH. Bundles downloaded with an explicit architecture argument won't show as cached in thelistoutput. Consider adding an optional arch parameter or usingBundleNamedirectly.-func isBundleCached(preset crcPreset.Preset, version string) bool { - bundleName := constants.BundleForPreset(preset, version) +func isBundleCached(preset crcPreset.Preset, version string, arch string) bool { + bundleName := constants.BundleName(preset, version, arch) bundlePath := filepath.Join(constants.MachineCacheDir, bundleName) if _, err := os.Stat(bundlePath); err == nil { return true } return false }
319-327: Consider more robust hash file parsing.The current parsing assumes exact format "hash filename". Using
strings.Fieldswould be more resilient to whitespace variations.lines := strings.Split(verifiedHashes, "\n") for _, line := range lines { - if strings.HasSuffix(line, bundleName) { - sha256sum := strings.TrimSuffix(line, " "+bundleName) - return sha256sum, nil + fields := strings.Fields(line) + if len(fields) == 2 && fields[1] == bundleName { + return fields[0], nil } }
333-346: Consider using context for request cancellation.The HTTP client has a timeout, but using
http.NewRequestWithContextwould allow callers to cancel long-running requests and integrate better with the rest of the codebase.-func fetchAvailableVersions(preset crcPreset.Preset) ([]*semver.Version, error) { +func fetchAvailableVersions(ctx context.Context, preset crcPreset.Preset) ([]*semver.Version, error) { // Base URL for the preset baseURL := fmt.Sprintf("https://mirror.openshift.com/pub/openshift-v4/clients/crc/bundles/%s/", preset.String()) client := &http.Client{ Transport: httpproxy.HTTPTransport(), Timeout: 10 * time.Second, } - req, err := http.NewRequest("GET", baseURL, nil) + req, err := http.NewRequestWithContext(ctx, "GET", baseURL, nil) if err != nil { return nil, err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Makefile(1 hunks)cmd/crc/cmd/bundle/bundle.go(1 hunks)cmd/crc/cmd/bundle/download.go(1 hunks)cmd/crc/cmd/root_test.go(1 hunks)pkg/crc/constants/constants.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/crc/cmd/bundle/bundle.go
- cmd/crc/cmd/root_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/crc/constants/constants.go (2)
pkg/crc/preset/preset.go (1)
Preset(9-9)pkg/crc/config/settings.go (1)
Preset(33-33)
cmd/crc/cmd/bundle/download.go (8)
pkg/crc/preset/preset.go (1)
ParsePresetE(51-59)pkg/crc/config/settings.go (1)
GetPreset(169-171)pkg/crc/constants/constants.go (3)
GetDefaultBundlePath(129-131)GetDefaultBundleDownloadURL(133-139)BundleName(82-105)pkg/crc/logging/logging.go (1)
Warnf(100-102)pkg/download/download.go (1)
InMemory(101-126)pkg/crc/gpg/gpg.go (1)
GetVerifiedClearsignedMsgV3(41-68)pkg/crc/constants/keys.go (1)
RedHatReleaseKey(22-49)vendor/github.com/sebrandon1/grab/lib/request.go (1)
NewRequest(113-125)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: verify-devcontainer
- GitHub Check: build (macOS-15-intel, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
🔇 Additional comments (6)
Makefile (1)
94-97: LGTM!The new
buildtarget provides a convenient way to compile without installing, using the same flags asinstall. This is useful for local development and CI workflows where installation isn't needed.pkg/crc/constants/constants.go (1)
78-105: LGTM!Good refactoring that extracts the architecture parameter while preserving backward compatibility through
BundleForPreset. This enables multi-architecture bundle support as needed by the new download command.cmd/crc/cmd/bundle/download.go (4)
29-60: LGTM!Well-structured command setup with proper flag handling and preset resolution. The subcommand organization follows Cobra best practices.
171-212: LGTM!The list implementation correctly handles version filtering and cache indication. The use of
fmt.Printffor the actual version list (vslogging.Infoffor informational messages) appropriately separates structured output from log messages.
223-297: Well-implemented download logic with proper security measures.Good implementation with:
- Cache-aware downloads with force override
- Version resolution for partial versions
- Mandatory signature verification before download
- SHA256 integrity check during download
The separation between default bundle (using existing
bundle.Download) and custom version downloads is appropriate.
404-436: LGTM!Clean version resolution logic with appropriate early returns. Finding the first match from the sorted (newest-first) list correctly resolves partial versions like
4.19to the latest patch4.19.x.
cmd/crc/cmd/bundle/download.go
Outdated
| cleared := false | ||
| for _, file := range files { | ||
| if strings.HasSuffix(file.Name(), ".crcbundle") { | ||
| filePath := filepath.Join(cacheDir, file.Name()) | ||
| logging.Infof("Deleting %s", filePath) | ||
| if err := os.RemoveAll(filePath); err != nil { | ||
| logging.Errorf("Failed to remove %s: %v", filePath, err) | ||
| } | ||
| cleared = true | ||
| } | ||
| } | ||
|
|
||
| if !cleared { | ||
| logging.Infof("No bundles found in %s", cacheDir) | ||
| } else { | ||
| logging.Infof("Cleared cached bundles in %s", cacheDir) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Partial deletion failures are silently swallowed.
If some bundles fail to delete (e.g., permission issues), the function logs errors but returns nil, indicating success. Consider collecting errors and returning them, or at least returning a non-nil error when any deletion fails.
cleared := false
+ var lastErr error
for _, file := range files {
if strings.HasSuffix(file.Name(), ".crcbundle") {
filePath := filepath.Join(cacheDir, file.Name())
logging.Infof("Deleting %s", filePath)
if err := os.RemoveAll(filePath); err != nil {
logging.Errorf("Failed to remove %s: %v", filePath, err)
+ lastErr = err
+ } else {
+ cleared = true
}
- cleared = true
}
}
if !cleared {
logging.Infof("No bundles found in %s", cacheDir)
} else {
logging.Infof("Cleared cached bundles in %s", cacheDir)
}
- return nil
+ return lastErr
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cleared := false | |
| for _, file := range files { | |
| if strings.HasSuffix(file.Name(), ".crcbundle") { | |
| filePath := filepath.Join(cacheDir, file.Name()) | |
| logging.Infof("Deleting %s", filePath) | |
| if err := os.RemoveAll(filePath); err != nil { | |
| logging.Errorf("Failed to remove %s: %v", filePath, err) | |
| } | |
| cleared = true | |
| } | |
| } | |
| if !cleared { | |
| logging.Infof("No bundles found in %s", cacheDir) | |
| } else { | |
| logging.Infof("Cleared cached bundles in %s", cacheDir) | |
| } | |
| return nil | |
| } | |
| cleared := false | |
| var lastErr error | |
| for _, file := range files { | |
| if strings.HasSuffix(file.Name(), ".crcbundle") { | |
| filePath := filepath.Join(cacheDir, file.Name()) | |
| logging.Infof("Deleting %s", filePath) | |
| if err := os.RemoveAll(filePath); err != nil { | |
| logging.Errorf("Failed to remove %s: %v", filePath, err) | |
| lastErr = err | |
| } else { | |
| cleared = true | |
| } | |
| } | |
| } | |
| if !cleared { | |
| logging.Infof("No bundles found in %s", cacheDir) | |
| } else { | |
| logging.Infof("Cleared cached bundles in %s", cacheDir) | |
| } | |
| return lastErr | |
| } |
🤖 Prompt for AI Agents
In cmd/crc/cmd/bundle/download.go around lines 108 to 126, the loop deletes
found .crcbundle files but swallows partial failures by only logging them and
still returning nil; change this to collect any deletion errors (e.g.,
accumulate errors in a slice or use a multi-error helper) and after the loop, if
any delete failed return a non-nil aggregated error (while preserving per-file
logging and the existing info logs), otherwise return nil; ensure the returned
error includes which file(s) failed and the underlying error(s) for easier
debugging.
|
@sebrandon1 Thanks for addressing my comments. If you could just squash the new commit into the previous one please :) Otherwise LGTM |
sebrandon1
force-pushed
the
add_bundle_download_cmd
branch
from
5f22c48 to
793e90c
Compare
|
Squashed ✅ |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cmd/crc/cmd/bundle/download.go (2)
96-126: runClear swallows deletion failures but still reports full successIf any
os.RemoveAllcall fails, the error is only logged andrunClearstill returnsnil, potentially printing “Cleared cached bundles” even when some bundles remain. This can mislead users and makes troubleshooting harder. (This was already raised in a previous review.)You can track whether any deletions failed and return a non‑nil error when they do, while keeping per‑file logging. For example:
func runClear() error { @@ - cleared := false + cleared := false + var lastErr error for _, file := range files { if strings.HasSuffix(file.Name(), ".crcbundle") { filePath := filepath.Join(cacheDir, file.Name()) logging.Infof("Deleting %s", filePath) if err := os.RemoveAll(filePath); err != nil { logging.Errorf("Failed to remove %s: %v", filePath, err) + lastErr = err + continue } cleared = true } } @@ } else { logging.Infof("Cleared cached bundles in %s", cacheDir) } - return nil + return lastErr }
128-169: Fix potential panic in runPrune sort comparator and surface deletion failuresThe sort comparator ignores errors from
DirEntry.Info()and then callsModTime()unconditionally. IfInfo()fails,FileInfocan benil, causing a panic. This was already pointed out earlier and is still present. Additionally, likerunClear, deletion failures are only logged andrunPrunestill returnsnil.You can make the sort and deletion logic safer, for example:
// Sort by modification time, newest first sort.Slice(bundleFiles, func(i, j int) bool { - infoI, _ := bundleFiles[i].Info() - infoJ, _ := bundleFiles[j].Info() - return infoI.ModTime().After(infoJ.ModTime()) + infoI, errI := bundleFiles[i].Info() + infoJ, errJ := bundleFiles[j].Info() + + switch { + case errI != nil && errJ != nil: + // Fallback to name ordering if both fail + return bundleFiles[i].Name() < bundleFiles[j].Name() + case errI != nil: + // Treat missing info as oldest + return false + case errJ != nil: + return true + default: + return infoI.ModTime().After(infoJ.ModTime()) + } }) @@ - for i := keep; i < len(bundleFiles); i++ { + var lastErr error + 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) + lastErr = err } } - return nil + return lastErr
🧹 Nitpick comments (5)
pkg/crc/constants/constants.go (1)
78-105: BundleName helper is a good extraction; OS is still hard-wired to runtime.GOOSThe refactor keeps existing behavior for
BundleForPresetwhile enabling explicit arch selection viaBundleName, which is what the new download workflow needs. If you ever need to construct bundle names for a non-local OS (e.g., generating docs or tooling for another platform), consider adding an OS parameter instead of relying onruntime.GOOS, but that’s not required for this PR.cmd/crc/cmd/bundle/download.go (4)
29-94: Command wiring and subcommands look cohesiveThe
downloadcommand structure withlist,clear, andprunesubcommands is clear and consistent with the rest of the CLI. Hard‑wiringpruneto keep 2 bundles is fine for now, but if you expect different retention policies, consider a--keepflag (defaulting to 2) to make this configurable without code changes.
171-221:listonly marks bundles cached for the default arch; multi‑arch caches aren’t reflected
runListusesisBundleCached(preset, v.String()), andisBundleCacheddelegates toconstants.BundleForPreset, which always usesruntime.GOARCH. If a user downloads a different architecture (e.g.,arm64on anamd64host), that version will never be shown as “(cached)” even though a bundle file exists.If multi‑arch usage is a target for this command, consider either:
- Scanning the cache directory for any file matching the version regardless of arch (e.g.,
crc_*_<version>_*.crcbundle) and marking the version as cached if any match exists, or- Extending
listto show per‑arch information explicitly.As a smaller UX tweak, you might also want to warn when a provided
listfilter argument cannot be parsed as eitherMajor.MinororMajor.Minor.Patch, instead of silently listing everything.
223-297: Download flow and version resolution look solid; minor logging nitThe control flow for default vs custom version, version resolution, and signature verification is coherent and aligns with the new
BundleNamehelper. One small clarity improvement: the “Destination” debug log currently printsconstants.MachineCacheDir, while the actual target isbundlePath/defaultBundlePath. Logging the full file path would be more precise when debugging user reports.- logging.Debugf("Destination: %s", constants.MachineCacheDir) + logging.Debugf("Destination: %s", bundlePath)(and similarly for the default‑bundle branch).
299-327: Hash-line parsing assumes a very specificsha256sumformat
getVerifiedHashForCustomVersiontrims" "+bundleNamefrom the end of the line to extract the hash. This works as long as the file uses exactly two spaces before the filename and no extra markers (like*), but it’s a bit brittle: any formatting change insha256sum.txtwill cause a failure to find the hash, even though the signature verifies.You could make this more robust by splitting on whitespace and matching the filename field, e.g.:
for _, line := range lines { - if strings.HasSuffix(line, bundleName) { - sha256sum := strings.TrimSuffix(line, " "+bundleName) - return sha256sum, nil - } + fields := strings.Fields(line) + if len(fields) >= 2 && fields[len(fields)-1] == bundleName { + return fields[0], nil + } }This keeps behavior the same but tolerates minor formatting changes in the checksum file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Makefile(1 hunks)cmd/crc/cmd/bundle/bundle.go(1 hunks)cmd/crc/cmd/bundle/download.go(1 hunks)cmd/crc/cmd/root_test.go(1 hunks)pkg/crc/constants/constants.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/crc/cmd/root_test.go
- cmd/crc/cmd/bundle/bundle.go
- Makefile
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/crc/constants/constants.go (2)
pkg/crc/preset/preset.go (1)
Preset(9-9)pkg/crc/config/settings.go (1)
Preset(33-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: build (macOS-15-intel, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
🔇 Additional comments (2)
cmd/crc/cmd/bundle/download.go (2)
329-402: Version discovery and sorting logic look correct
fetchAvailableVersionsuses a bounded‑timeout client, scrapes onlyx.y.z-shaped paths, deduplicates them, and returns versions sorted newest‑first, which pairs nicely withresolveOpenShiftVersionand thelistcommand. No functional issues spotted here.
404-436: Partial-version resolution to latest patch is implemented correctly
resolveOpenShiftVersioncleanly handles full versions,Major.Minorshorthand, and “other” strings (tags) by returning them unchanged, while the partial‑version path walks the sorted versions to pick the latest matching patch. Error handling (mirror failure / no match) is clear and surfaces useful messages torunDownload.
albfan
left a comment
albfan
left a comment
There was a problem hiding this comment.
Add a better description on commands added, you can split into commits for easy cherry-picking
Consider split commands into different files (now all is in download.go)
is not clear if users will expect crc bundle download prune or just crc bundle prune
sebrandon1
force-pushed
the
add_bundle_download_cmd
branch
from
793e90c to
a0cc564
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cmd/crc/cmd/bundle/clear.go (1)
36-46: Partial deletion failures are silently swallowed and incorrectly counted as cleared.The
clearedflag is set totrueeven whenos.RemoveAllfails. This means:
- The function returns
nileven when some deletions failed- The success message is shown despite failures
cleared := false + var lastErr error for _, file := range files { if strings.HasSuffix(file.Name(), ".crcbundle") { filePath := filepath.Join(cacheDir, file.Name()) logging.Infof("Deleting %s", filePath) if err := os.RemoveAll(filePath); err != nil { logging.Errorf("Failed to remove %s: %v", filePath, err) + lastErr = err + } else { + cleared = true } - cleared = true } } if !cleared { logging.Infof("No bundles found in %s", cacheDir) } else { logging.Infof("Cleared cached bundles in %s", cacheDir) } - return nil + return lastErrcmd/crc/cmd/bundle/prune.go (1)
50-55: Potential nil pointer dereference whenInfo()fails.If
Info()returns an error (e.g., file deleted betweenReadDirandInfocall, or permission issue), the returnedFileInfoisnil, causing a panic when accessingModTime().// Sort by modification time, newest first sort.Slice(bundleFiles, func(i, j int) bool { - infoI, _ := bundleFiles[i].Info() - infoJ, _ := bundleFiles[j].Info() + 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 + } return infoI.ModTime().After(infoJ.ModTime()) })
🧹 Nitpick comments (6)
cmd/crc/cmd/bundle/list.go (1)
36-46: Consider informing the user when filter parsing fails.When an argument is provided but cannot be parsed as a valid version (neither as partial nor full), the
filterremainsniland all versions are displayed without any indication to the user that their filter was ignored.var filter *semver.Version if len(args) > 0 { v, err := semver.NewVersion(args[0] + ".0") // Treat 4.19 as 4.19.0 for partial matching if err == nil { filter = v } else { // Try parsing as full version just in case v, err = semver.NewVersion(args[0]) if err == nil { filter = v + } else { + logging.Warnf("Invalid version filter %q, showing all versions", args[0]) } } }cmd/crc/cmd/bundle/prune.go (1)
14-24: Consider makingkeepconfigurable via a flag.The
keepvalue is hardcoded to 2. Users may want to retain a different number of bundles.func getPruneCmd() *cobra.Command { - return &cobra.Command{ + cmd := &cobra.Command{ Use: "prune", Short: "Prune old CRC bundles", Long: "Keep only the most recent bundles and delete older ones to save space.", RunE: func(cmd *cobra.Command, args []string) error { - // Default keep 2 most recent - return runPrune(2) + keep, _ := cmd.Flags().GetInt("keep") + return runPrune(keep) }, } + cmd.Flags().IntP("keep", "k", 2, "Number of most recent bundles to keep") + return cmd }cmd/crc/cmd/bundle/download.go (1)
148-154: Hash parsing may fail silently if format differs.The hash extraction assumes the sha256sum format with exactly two spaces between hash and filename (
"hash filename"). If the format varies (e.g., single space, or*prefix for binary mode), the parsing will fail to match or extract incorrectly.lines := strings.Split(verifiedHashes, "\n") for _, line := range lines { - if strings.HasSuffix(line, bundleName) { - sha256sum := strings.TrimSuffix(line, " "+bundleName) - return sha256sum, nil + // sha256sum format: "hash filename" or "hash *filename" (binary mode) + line = strings.TrimSpace(line) + if strings.HasSuffix(line, bundleName) || strings.HasSuffix(line, "*"+bundleName) { + fields := strings.Fields(line) + if len(fields) >= 2 { + return fields[0], nil + } } }cmd/crc/cmd/bundle/util.go (3)
26-26: Consider extracting regex patterns and making timeout configurable.Optional improvements for maintainability:
- Extract the regex patterns (lines 50, 73) as package-level constants for easier testing and maintenance.
- Consider making the 10-second timeout configurable via a constant or parameter for users with slow network connections.
Example refactor:
+const ( + mirrorFetchTimeout = 10 * time.Second + versionHrefPattern = `href=["']?\.?/?(\d+\.\d+\.\d+)/?["']?` + versionSimplePattern = `>(\d+\.\d+\.\d+)/?<` +) + func fetchAvailableVersions(preset crcPreset.Preset) ([]*semver.Version, error) { // Base URL for the preset baseURL := fmt.Sprintf("%s/%s/", constants.DefaultMirrorURL, preset.String()) client := &http.Client{ Transport: httpproxy.HTTPTransport(), - Timeout: 10 * time.Second, + Timeout: mirrorFetchTimeout, } // ... request handling ... // Parse the HTML directory listing to find version directories - versionRegex := regexp.MustCompile(`href=["']?\.?/?(\d+\.\d+\.\d+)/?["']?`) + versionRegex := regexp.MustCompile(versionHrefPattern) // ... matching logic ... // If regex failed, try a simpler one for directory names in text if len(versions) == 0 { - simpleRegex := regexp.MustCompile(`>(\d+\.\d+\.\d+)/?<`) + simpleRegex := regexp.MustCompile(versionSimplePattern)Also applies to: 50-50, 73-73
103-106: Non-standard version formats are passed through without validation.When the input doesn't match
Major.Minor.PatchorMajor.Minorpatterns, the function returns it as-is (line 105). This could lead to errors downstream if users provide malformed versions like"4.x","latest", or"4.19-rc1".Consider validating these cases or documenting that arbitrary strings are accepted (e.g., for tags).
Apply this diff to add validation for common edge cases:
// If not Major.Minor, return as is (could be a tag or other format user intends) partialVersionRegex := regexp.MustCompile(`^(\d+\.\d+)$`) if !partialVersionRegex.MatchString(inputVersion) { - return inputVersion, nil + // Check if it's at least a valid-looking tag/version string + if matched, _ := regexp.MatchString(`^[\w][\w.-]*$`, inputVersion); !matched { + return "", fmt.Errorf("invalid version format: %s", inputVersion) + } + logging.Debugf("Using version string as-is: %s", inputVersion) + return inputVersion, nil }Alternatively, document in a comment that arbitrary version strings are accepted for flexibility with tags.
129-136: Improve error handling to distinguish between "not cached" and other errors.The function returns
falsefor anyos.Staterror, which masks issues like permission denied or corrupted filesystem paths. This could make debugging difficult if cache directory permissions are incorrect.Apply this diff to be more explicit about error types:
+ "errors" + func isBundleCached(preset crcPreset.Preset, version string) bool { bundleName := constants.BundleForPreset(preset, version) bundlePath := filepath.Join(constants.MachineCacheDir, bundleName) - if _, err := os.Stat(bundlePath); err == nil { - return true + _, err := os.Stat(bundlePath) + if err == nil { + return true + } + if errors.Is(err, os.ErrNotExist) { + return false } + // Log unexpected errors but treat as not cached + logging.Debugf("Error checking bundle cache: %v", err) return false }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/crc/cmd/bundle/bundle.go(1 hunks)cmd/crc/cmd/bundle/clear.go(1 hunks)cmd/crc/cmd/bundle/download.go(1 hunks)cmd/crc/cmd/bundle/list.go(1 hunks)cmd/crc/cmd/bundle/prune.go(1 hunks)cmd/crc/cmd/bundle/util.go(1 hunks)cmd/crc/cmd/root_test.go(1 hunks)pkg/crc/constants/constants.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
cmd/crc/cmd/bundle/list.go (2)
pkg/crc/config/settings.go (1)
GetPreset(147-149)pkg/crc/logging/logging.go (1)
Infof(92-94)
cmd/crc/cmd/bundle/download.go (6)
pkg/crc/preset/preset.go (1)
ParsePresetE(51-59)pkg/crc/constants/constants.go (5)
GetDefaultBundlePath(135-137)GetDefaultBundleDownloadURL(139-145)BundleName(88-111)MachineCacheDir(128-128)DefaultMirrorURL(33-33)pkg/crc/logging/logging.go (3)
Infof(92-94)Debugf(124-126)Warnf(100-102)pkg/download/download.go (1)
InMemory(101-126)pkg/crc/gpg/gpg.go (1)
GetVerifiedClearsignedMsgV3(41-68)pkg/crc/constants/keys.go (1)
RedHatReleaseKey(22-49)
cmd/crc/cmd/bundle/prune.go (2)
pkg/crc/constants/constants.go (1)
MachineCacheDir(128-128)pkg/crc/logging/logging.go (1)
Infof(92-94)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: build (macOS-15-intel, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-15, 1.24)
🔇 Additional comments (9)
cmd/crc/cmd/bundle/bundle.go (1)
12-21: LGTM!The new subcommands are wired correctly following the existing pattern used for
getGenerateCmd. The updated long description accurately reflects the expanded functionality.cmd/crc/cmd/bundle/list.go (1)
12-21: LGTM!The command definition is clean with appropriate usage documentation.
pkg/crc/constants/constants.go (2)
88-111: LGTM!The
BundleNamefunction correctly separates the OS-specific hypervisor suffix (derived fromruntime.GOOS) from the architecture parameter, enabling cross-architecture bundle downloads while maintaining the correct hypervisor naming for the current platform.
33-34: LGTM!Extracting
DefaultMirrorURLas a separate constant improves reusability and makes the URL construction inDefaultBundleURLBaseclearer.cmd/crc/cmd/bundle/download.go (3)
23-50: LGTM!The command setup with
--forceand--presetflags is well-structured. The preset parsing with proper error handling is good practice.
74-84: LGTM!Good fallback behavior when version resolution fails - logs a warning and continues with the original version string rather than failing outright.
67-71: Remove incorrect concern about verification parameter.The third parameter is
enableBundleQuayFallback, notenableBundleVerification. Setting it tofalseprevents falling back to quay if the mirror download fails, which is the correct behavior for default bundles. The comment accurately describes that verification is handled internally—downloadDefaultcallsgetDefaultBundleVerifiedHash, which verifies the GPG signature on the hash file, and then uses that verified hash during download.Likely an incorrect or invalid review comment.
cmd/crc/cmd/bundle/util.go (2)
1-18: LGTM!The package declaration and imports are well-organized and appropriate for the bundle management utilities.
22-22: Primary regex is dead code; fallback regex successfully extracts versions but both remain fragile if mirror HTML format changes.The function uses regex to parse the mirror's HTML directory listing. The primary regex pattern (line 50) returns no matches against the current mirror HTML—which wraps versions in
<span class="name">VERSION</span>tags. The fallback regex (line 73) correctly extracts all 35+ available versions from the>VERSION<pattern. While the fallback currently works reliably, both patterns are fragile and would fail if the mirror changes its HTML structure. The primary regex should be removed as it serves no purpose; the fallback alone should be retained but documented as the only extraction pattern.
|
See the mileage for reorder and split this PR is not important, don't add commits with "split, comments" that's the internal history for this PR, the final commits needs to reflect what each commit is doing:
Some new subcommands will be easier to approve than others, helping to merge all this. That requires squash all the commits and split changes again |
sebrandon1
force-pushed
the
add_bundle_download_cmd
branch
from
a0cc564 to
d09a477
Compare
|
Okay I have squashed my commits and I have removed the |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/crc/cmd/root_test.go (1)
26-29: Critical: Manpage test expectations still don't match the command structure.Despite the past review comment being marked as addressed, the issue persists. These manpage names imply
clear,list, andpruneare subcommands ofdownload(e.g.,crc bundle download clear). However, inbundle.go(lines 18-21), these commands are added as direct subcommands ofbundleviabundleCmd.AddCommand(), making them peers ofdownloadrather than its children.The correct manpage names should be:
crc-bundle-clear.1(notcrc-bundle-download-clear.1)crc-bundle-list.1(notcrc-bundle-download-list.1)crc-bundle-prune.1(notcrc-bundle-download-prune.1)crc-bundle-download.1✓ (this one is correct)🔎 Apply this diff to fix the manpage expectations:
assert.ElementsMatch(t, []string{ - "crc-bundle-download-clear.1", - "crc-bundle-download-list.1", - "crc-bundle-download-prune.1", + "crc-bundle-clear.1", + "crc-bundle-list.1", + "crc-bundle-prune.1", "crc-bundle-download.1",
🧹 Nitpick comments (2)
cmd/crc/cmd/bundle/util.go (2)
24-27: Consider increasing the HTTP timeout for slower connections.The 10-second timeout might be insufficient for users on slow or unstable network connections, potentially causing false negatives when fetching version listings from the mirror.
🔎 Consider increasing the timeout:
client := &http.Client{ Transport: httpproxy.HTTPTransport(), - Timeout: 10 * time.Second, + Timeout: 30 * time.Second, }
49-88: Replace fragile regex HTML parsing with a proper HTML parser.The code parses HTML directory listings from mirror.openshift.com using regex patterns, which is fragile and vulnerable to format changes. Mirror.openshift.com provides directory listings in HTML format, not a structured API. While a fallback regex is provided, both patterns could fail if the HTML structure changes.
Consider using Goquery, a popular HTML parsing library for Go with jQuery-like syntax that performs HTML document traversals and selections efficiently using CSS selectors, or Go's standard net/html package for parsing and manipulating HTML documents in a structured manner.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/crc/cmd/bundle/bundle.go(1 hunks)cmd/crc/cmd/bundle/clear.go(1 hunks)cmd/crc/cmd/bundle/download.go(1 hunks)cmd/crc/cmd/bundle/list.go(1 hunks)cmd/crc/cmd/bundle/prune.go(1 hunks)cmd/crc/cmd/bundle/util.go(1 hunks)cmd/crc/cmd/root_test.go(1 hunks)pkg/crc/constants/constants.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/crc/cmd/bundle/clear.go
- cmd/crc/cmd/bundle/download.go
- cmd/crc/cmd/bundle/prune.go
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/crc/constants/constants.go (2)
pkg/crc/config/settings.go (1)
Preset(32-32)pkg/crc/preset/preset.go (1)
Preset(9-9)
cmd/crc/cmd/bundle/util.go (3)
pkg/crc/constants/constants.go (3)
DefaultMirrorURL(33-33)BundleForPreset(84-86)MachineCacheDir(128-128)pkg/crc/network/httpproxy/proxy.go (1)
HTTPTransport(257-264)pkg/crc/logging/logging.go (1)
Debugf(124-126)
cmd/crc/cmd/bundle/list.go (2)
pkg/crc/config/settings.go (1)
GetPreset(147-149)pkg/crc/logging/logging.go (1)
Infof(92-94)
🔇 Additional comments (7)
cmd/crc/cmd/bundle/bundle.go (2)
12-12: LGTM!The updated long description accurately reflects the new bundle management capabilities.
18-21: LGTM!The four new subcommands are properly wired as direct children of the bundle command, expanding the CLI's bundle management capabilities.
cmd/crc/cmd/bundle/list.go (1)
57-61: LGTM!The cache status annotation provides helpful user feedback, clearly indicating which bundles are already downloaded.
pkg/crc/constants/constants.go (2)
33-34: LGTM!Centralizing the mirror URL as
DefaultMirrorURLand derivingDefaultBundleURLBasefrom it improves maintainability and makes it easier to change the mirror location in the future.
84-111: LGTM!The refactoring to introduce
BundleNamewith an explicitarchparameter enables cross-architecture bundle name generation while maintaining backward compatibility throughBundleForPreset, which delegates toBundleNamewithruntime.GOARCH.cmd/crc/cmd/bundle/util.go (2)
95-127: Verify the fallback behavior for non-standard version formats.Lines 103-106 return the input version as-is if it doesn't match the Major.Minor pattern. This means inputs like "latest", "4.19.x", or other non-standard formats are passed through without validation. While this might be intentional to support tags or other formats, it could lead to confusing errors downstream when attempting to construct URLs or filenames.
Consider whether explicit validation and error handling for unsupported formats would improve the user experience.
129-136: LGTM!The cache check is straightforward and correctly uses
os.Statto determine file existence.
sebrandon1
force-pushed
the
add_bundle_download_cmd
branch
from
d09a477 to
779e3b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @cmd/crc/cmd/bundle/clear.go:
- Around line 36-46: The loop in clear.go sets cleared = true even when
os.RemoveAll fails and still returns nil; change the logic so that cleared is
only set to true after a successful os.RemoveAll, and make the function return
an error when any deletion fails (or aggregate errors) instead of swallowing
failures. Specifically, in the for loop over files (check for
strings.HasSuffix(file.Name(), ".crcbundle")), move cleared = true to after the
os.RemoveAll succeeds, and on error from os.RemoveAll collect or return that
error (e.g., return fmt.Errorf or append to a slice and return a combined error)
so callers can detect failures. Ensure logging remains but do not treat logged
failures as success.
🧹 Nitpick comments (3)
cmd/crc/cmd/bundle/prune.go (1)
14-23: Consider exposingkeepas a CLI flag.The
keepparameter is hardcoded to 2. For flexibility, consider adding a--keepflag so users can specify how many bundles to retain.🔎 Example implementation
func getPruneCmd() *cobra.Command { - return &cobra.Command{ + cmd := &cobra.Command{ Use: "prune", Short: "Prune old CRC bundles", Long: "Keep only the most recent bundles and delete older ones to save space.", RunE: func(cmd *cobra.Command, args []string) error { - // Default keep 2 most recent - return runPrune(2) + keep, _ := cmd.Flags().GetInt("keep") + return runPrune(keep) }, } + cmd.Flags().IntP("keep", "k", 2, "Number of most recent bundles to keep") + return cmd }cmd/crc/cmd/bundle/download.go (2)
28-30: Ignoring flag retrieval errors may hide configuration issues.The errors from
GetBoolandGetStringare discarded. While these typically don't fail for registered flags, silently ignoring errors can hide misconfigurations during development or refactoring.🔎 Consider logging or handling errors
RunE: func(cmd *cobra.Command, args []string) error { - force, _ := cmd.Flags().GetBool("force") - presetStr, _ := cmd.Flags().GetString("preset") + force, err := cmd.Flags().GetBool("force") + if err != nil { + return fmt.Errorf("failed to get force flag: %w", err) + } + presetStr, err := cmd.Flags().GetString("preset") + if err != nil { + return fmt.Errorf("failed to get preset flag: %w", err) + }
86-89: Consider validating the architecture argument.The architecture value from
args[1]is used directly without validation. While an invalid value will result in a download failure (404), validating against known architectures (e.g.,amd64,arm64) would provide a clearer error message.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/crc/cmd/bundle/bundle.gocmd/crc/cmd/bundle/clear.gocmd/crc/cmd/bundle/download.gocmd/crc/cmd/bundle/list.gocmd/crc/cmd/bundle/prune.gocmd/crc/cmd/bundle/util.gocmd/crc/cmd/root_test.gopkg/crc/constants/constants.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/crc/cmd/bundle/util.go
- cmd/crc/cmd/root_test.go
- pkg/crc/constants/constants.go
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/crc/cmd/bundle/list.go (2)
pkg/crc/config/settings.go (1)
GetPreset(147-149)pkg/crc/logging/logging.go (1)
Infof(92-94)
cmd/crc/cmd/bundle/clear.go (2)
pkg/crc/constants/constants.go (1)
MachineCacheDir(128-128)pkg/crc/logging/logging.go (1)
Infof(92-94)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build (macOS-15-intel, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
🔇 Additional comments (4)
cmd/crc/cmd/bundle/bundle.go (1)
17-21: LGTM!The new subcommands are wired consistently with the existing
getGenerateCmdpattern. The updated Long description accurately reflects the expanded functionality.cmd/crc/cmd/bundle/list.go (1)
49-62: LGTM!The output pattern of logging the header via
logging.Infof(stderr) and printing versions viafmt.Printf(stdout) is appropriate for CLI tools, enabling clean piping of version data.cmd/crc/cmd/bundle/download.go (2)
128-156: Verification logic looks sound.The GPG signature verification using
RedHatReleaseKeyand hash extraction follow the standard sha256sum file format correctly. TheHasSuffixcheck is appropriate since bundle names are specific enough to avoid false matches.
69-71: The default bundle download properly handles signature verification. The fourth parameter isenableBundleQuayFallback(not related to verification), and verification happens through two mechanisms:
- GPG signature verification of the signed hash file (via
getVerifiedHashusingRedHatReleaseKey)- SHA256 verification of the downloaded bundle content (via the
Downloadfunction with the verified hash)The comment on line 69 is accurate.
sebrandon1
force-pushed
the
add_bundle_download_cmd
branch
2 times, most recently
from
132bd85 to
3f865b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @cmd/crc/cmd/bundle/util.go:
- Around line 129-136: isBundleCached currently uses constants.BundleForPreset
which calls BundleName with runtime.GOARCH, so it will miss cached bundles for
other architectures; change isBundleCached(preset crcPreset.Preset, version
string) to isBundleCached(preset crcPreset.Preset, version, arch string) and
build the bundle filename with constants.BundleName(preset, version, arch) (or
the appropriate signature that accepts arch) before checking
filepath.Join(constants.MachineCacheDir, bundleName) and os.Stat; update all
callers of isBundleCached to pass the requested arch.
🧹 Nitpick comments (2)
cmd/crc/cmd/bundle/util.go (2)
20-93: Robust implementation with good fallback logic.The function properly handles proxy configuration, timeout, error cases, and has a fallback regex when the primary pattern fails. The deduplication and sorting logic is correct.
Consider adding a debug log when falling back to the simpler regex to aid troubleshooting:
🔎 Optional enhancement
// If regex failed, try a simpler one for directory names in text if len(versions) == 0 { + logging.Debugf("Primary regex found no versions, trying fallback pattern") simpleRegex := regexp.MustCompile(`>(\d+\.\d+\.\d+)/?<`)
95-127: Function name is misleading.The function is named
resolveOpenShiftVersionbut it works for all presets (OpenShift, OKD, Microshift), not just OpenShift. Consider renaming toresolveVersionorresolveBundleVersionfor clarity.🔎 Suggested rename
-func resolveOpenShiftVersion(preset crcPreset.Preset, inputVersion string) (string, error) { +func resolveVersion(preset crcPreset.Preset, inputVersion string) (string, error) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/crc/cmd/bundle/bundle.gocmd/crc/cmd/bundle/clear.gocmd/crc/cmd/bundle/download.gocmd/crc/cmd/bundle/list.gocmd/crc/cmd/bundle/prune.gocmd/crc/cmd/bundle/util.gocmd/crc/cmd/root_test.gopkg/crc/constants/constants.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/crc/cmd/bundle/download.go
- cmd/crc/cmd/root_test.go
- cmd/crc/cmd/bundle/clear.go
- cmd/crc/cmd/bundle/list.go
- cmd/crc/cmd/bundle/prune.go
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/crc/cmd/bundle/util.go (3)
pkg/crc/constants/constants.go (3)
DefaultMirrorURL(33-33)BundleForPreset(84-86)MachineCacheDir(128-128)pkg/crc/network/httpproxy/proxy.go (1)
HTTPTransport(257-264)pkg/crc/logging/logging.go (1)
Debugf(124-126)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-qe (darwin, arm64)
🔇 Additional comments (3)
pkg/crc/constants/constants.go (2)
33-34: Good refactoring to centralize the mirror URL.Extracting
DefaultMirrorURLas a constant and reusing it inDefaultBundleURLBaseimproves maintainability.
84-111: Clarify the design intent: arch parameter vs. runtime.GOOS usage.The new
BundleNamefunction accepts an explicitarchparameter, suggesting it can generate bundle names for arbitrary architectures. However, it still usesruntime.GOOSat line 100 to determine OS-specific hypervisor suffixes (_vfkit, _libvirt, _hyperv). This creates an asymmetry:
- The architecture is parameterized (allowing cross-arch name generation)
- The OS remains fixed to the current runtime environment
This could be confusing for callers expecting to generate bundle names for different OS/arch combinations. Please clarify whether:
- This is intentional (bundles are always OS-specific to the current runtime, but arch can vary)
- The
osshould also be parameterized for consistency- Documentation should be added explaining this behavior
Consider adding a function comment documenting this behavior:
// BundleName generates a bundle filename for the given preset, version, and architecture. // The hypervisor suffix (_vfkit, _libvirt, _hyperv) is determined by runtime.GOOS, // as bundles are OS-specific but may target different architectures. func BundleName(preset crcpreset.Preset, version string, arch string) string {cmd/crc/cmd/bundle/bundle.go (1)
12-12: LGTM! Clean integration of new subcommands.The updated description accurately reflects the expanded functionality, and the new subcommands follow the existing pattern used by
getGenerateCmd.Also applies to: 18-21
cmd/crc/cmd/bundle/util.go
Outdated
| func isBundleCached(preset crcPreset.Preset, version string) bool { | ||
| bundleName := constants.BundleForPreset(preset, version) | ||
| bundlePath := filepath.Join(constants.MachineCacheDir, bundleName) | ||
| if _, err := os.Stat(bundlePath); err == nil { | ||
| return true | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Cache check may not work correctly for cross-architecture bundles.
The function uses constants.BundleForPreset (line 130), which internally calls BundleName with runtime.GOARCH. This means isBundleCached can only check for bundles matching the current system's architecture.
According to the PR objectives, the feature supports downloading bundles for specific architectures: crc bundle download <version> <arch>. If users download a bundle for a different architecture (e.g., downloading arm64 bundles on an amd64 system), this function will not correctly detect them as cached.
Consider adding an architecture parameter or using constants.BundleName with an explicit arch argument:
🔎 Proposed fix
-func isBundleCached(preset crcPreset.Preset, version string) bool {
- bundleName := constants.BundleForPreset(preset, version)
+func isBundleCached(preset crcPreset.Preset, version string, arch string) bool {
+ bundleName := constants.BundleName(preset, version, arch)
bundlePath := filepath.Join(constants.MachineCacheDir, bundleName)
if _, err := os.Stat(bundlePath); err == nil {
return true
}
return false
}Then update all callers to pass the appropriate architecture parameter.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @cmd/crc/cmd/bundle/util.go around lines 129 - 136, isBundleCached currently
uses constants.BundleForPreset which calls BundleName with runtime.GOARCH, so it
will miss cached bundles for other architectures; change isBundleCached(preset
crcPreset.Preset, version string) to isBundleCached(preset crcPreset.Preset,
version, arch string) and build the bundle filename with
constants.BundleName(preset, version, arch) (or the appropriate signature that
accepts arch) before checking filepath.Join(constants.MachineCacheDir,
bundleName) and os.Stat; update all callers of isBundleCached to pass the
requested arch.
3f865b6 to
72c1c46
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/crc/cmd/bundle/download.go (1)
148-155: Hash extraction could be more robust.The current parsing assumes the exact format
hash filenamewith two spaces. Consider usingstrings.Fieldsto split on whitespace, which handles varying whitespace and validates that exactly two fields exist.🔎 Proposed fix
lines := strings.Split(verifiedHashes, "\n") for _, line := range lines { - if strings.HasSuffix(line, bundleName) { - sha256sum := strings.TrimSuffix(line, " "+bundleName) - return sha256sum, nil + fields := strings.Fields(line) + if len(fields) == 2 && fields[1] == bundleName { + return fields[0], nil } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/crc/cmd/bundle/bundle.gocmd/crc/cmd/bundle/clear.gocmd/crc/cmd/bundle/download.gocmd/crc/cmd/bundle/list.gocmd/crc/cmd/bundle/prune.gocmd/crc/cmd/bundle/util.gocmd/crc/cmd/root_test.gopkg/crc/constants/constants.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/crc/cmd/bundle/prune.go
- pkg/crc/constants/constants.go
- cmd/crc/cmd/bundle/list.go
- cmd/crc/cmd/root_test.go
🧰 Additional context used
🧬 Code graph analysis (3)
cmd/crc/cmd/bundle/download.go (5)
pkg/crc/config/settings.go (1)
GetPreset(147-149)pkg/crc/constants/constants.go (5)
GetDefaultBundlePath(135-137)GetDefaultBundleDownloadURL(139-145)BundleName(88-111)MachineCacheDir(128-128)DefaultMirrorURL(33-33)pkg/download/download.go (1)
InMemory(101-126)pkg/crc/gpg/gpg.go (1)
GetVerifiedClearsignedMsgV3(41-68)pkg/crc/constants/keys.go (1)
RedHatReleaseKey(22-49)
cmd/crc/cmd/bundle/clear.go (2)
pkg/crc/constants/constants.go (1)
MachineCacheDir(128-128)pkg/crc/logging/logging.go (1)
Infof(92-94)
cmd/crc/cmd/bundle/util.go (3)
pkg/crc/constants/constants.go (3)
DefaultMirrorURL(33-33)BundleName(88-111)MachineCacheDir(128-128)pkg/crc/network/httpproxy/proxy.go (1)
HTTPTransport(257-264)pkg/crc/logging/logging.go (1)
Debugf(124-126)
🔇 Additional comments (7)
cmd/crc/cmd/bundle/bundle.go (1)
8-22: LGTM!The bundle command wiring is clean and correctly integrates the new subcommands. The updated description accurately reflects the expanded functionality.
cmd/crc/cmd/bundle/clear.go (1)
24-57: LGTM!The
runClearimplementation correctly handles the deletion workflow with proper error tracking. The previous issue about silently swallowed deletion failures has been addressed—lastErrnow captures failures,clearedis only set on successful deletions, and the function returns the last error encountered.cmd/crc/cmd/bundle/download.go (2)
23-50: LGTM!The command structure is well-designed with appropriate flags for
--forceand--preset. The preset parsing includes proper error handling, and the default preset is correctly retrieved from config when not specified.
52-126: Well-structured download workflow with proper verification.The implementation correctly:
- Handles both default and custom version downloads
- Resolves partial versions (Major.Minor) to full versions
- Verifies bundle signatures before download
- Uses SHA256 integrity checking
The fallback behavior on version resolution failure (line 80) is reasonable—it warns and attempts with the original string, letting the download fail naturally if the URL is invalid.
cmd/crc/cmd/bundle/util.go (3)
20-93: Well-implemented version discovery with graceful fallback.The function properly:
- Uses proxy-aware HTTP transport with a reasonable timeout
- Handles HTTP errors appropriately
- Employs a primary regex with a simpler fallback for different HTML formats
- Deduplicates and validates versions using semver
- Returns versions sorted newest-first
95-127: Version resolution logic handles edge cases well.The function correctly:
- Passes through full versions unchanged
- Passes through non-standard formats (tags, etc.) unchanged
- Resolves Major.Minor to the latest patch version
- Returns a clear error when no matching version is found
129-136: LGTM!The previous review concern about cross-architecture support has been addressed. The function now accepts an
archparameter and correctly usesconstants.BundleName(preset, version, arch)to construct the bundle path.
|
/retest-required |
Address comments 1 Address comments; consts, split bundle commands into files
sebrandon1
force-pushed
the
add_bundle_download_cmd
branch
from
72c1c46 to
1b2454f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@cmd/crc/cmd/bundle/download.go`:
- Around line 52-66: The runDownload function currently ignores extra positional
args; add a guard at the start of runDownload that checks args length and
returns an error when len(args) > 2 (allowed: [version] [architecture]) so
callers fail fast with a clear message; update the error return to use
fmt.Errorf or the package's preferred error/logging pattern and reference
runDownload and args in the message to make it obvious which call site and
parameters were invalid.
- Around line 77-84: The code currently swallows errors from
resolveOpenShiftVersion and continues with the original partial version; change
this to fail fast: when resolveOpenShiftVersion(preset, version) returns an
error, propagate or return that error instead of logging a warning and
continuing. Update the logic around resolveOpenShiftVersion, logging.Warnf, and
the subsequent version assignment so that resolve errors cause an early return
(or a wrapped error) so callers know resolution failed rather than proceeding
with the unresolved partial version.
In `@cmd/crc/cmd/bundle/prune.go`:
- Around line 45-68: The prune logic currently sorts bundleFiles globally and
removes everything beyond the newest keep, but you must instead group bundles by
major.minor (and arch/preset if applicable) and prune within each group; update
the code around bundleFiles, sort.Slice and the removal loop to (1) parse each
file.Name() (or bundle metadata via file.Info()) to extract semver major.minor
and arch, (2) build a map[groupKey][]os.FileInfo (or []fs.File) where groupKey =
major.minor + ":" + arch, (3) for each group sort by semantic version (or
ModTime if version not parseable) to get newest-first, (4) keep the first keep
entries per group and call os.RemoveAll(filepath.Join(cacheDir, file.Name()))
only for the rest, and ensure errors from Info()/version parsing are handled
(treat unparsable entries as oldest so they get pruned last).
| 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 no args, use default bundle path | ||
| if len(args) == 0 { | ||
| defaultBundlePath := constants.GetDefaultBundlePath(preset) | ||
| if !force { | ||
| if _, err := os.Stat(defaultBundlePath); err == nil { | ||
| logging.Infof("Bundle %s already exists. Use --force to overwrite.", defaultBundlePath) | ||
| return nil | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Reject extra positional args to avoid silent ignores.
Right now, additional args beyond [version] [architecture] are ignored. Consider failing fast with a clear error.
✅ Proposed fix
func runDownload(args []string, preset crcPreset.Preset, force bool) error {
+ if len(args) > 2 {
+ return fmt.Errorf("too many arguments: expected at most 2 (version, architecture), got %d", len(args))
+ }
// 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.🤖 Prompt for AI Agents
In `@cmd/crc/cmd/bundle/download.go` around lines 52 - 66, The runDownload
function currently ignores extra positional args; add a guard at the start of
runDownload that checks args length and returns an error when len(args) > 2
(allowed: [version] [architecture]) so callers fail fast with a clear message;
update the error return to use fmt.Errorf or the package's preferred
error/logging pattern and reference runDownload and args in the message to make
it obvious which call site and parameters were invalid.
| // 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) | ||
| } else if resolvedVersion != version { | ||
| logging.Debugf("Resolved version %s to %s", version, resolvedVersion) | ||
| version = resolvedVersion | ||
| } |
There was a problem hiding this comment.
Fail fast when partial-version resolution fails.
For a major.minor input, continuing with an unresolved version will usually 404 or fail signature validation, which is less actionable than a clear resolution error.
✅ Proposed fix
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
}🤖 Prompt for AI Agents
In `@cmd/crc/cmd/bundle/download.go` around lines 77 - 84, The code currently
swallows errors from resolveOpenShiftVersion and continues with the original
partial version; change this to fail fast: when resolveOpenShiftVersion(preset,
version) returns an error, propagate or return that error instead of logging a
warning and continuing. Update the logic around resolveOpenShiftVersion,
logging.Warnf, and the subsequent version assignment so that resolve errors
cause an early return (or a wrapped error) so callers know resolution failed
rather than proceeding with the unresolved partial version.
| if len(bundleFiles) <= keep { | ||
| logging.Infof("Nothing to prune (found %d bundles, keeping %d)", len(bundleFiles), keep) | ||
| return nil | ||
| } | ||
|
|
||
| // 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 | ||
| } | ||
| 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
Prune logic keeps only N total bundles, not N per major.minor.
The current mtime-based pruning deletes everything beyond the newest keep bundles globally, which conflicts with the requirement to keep the latest two patch versions per major.minor (and can unintentionally delete other arch/preset bundles). Consider grouping by major.minor (and arch) and pruning within each group based on version.
🔧 Proposed fix (group by version/arch and prune within each group)
@@
- if len(bundleFiles) <= keep {
+ if len(bundleFiles) <= keep {
logging.Infof("Nothing to prune (found %d bundles, keeping %d)", len(bundleFiles), keep)
return nil
}
- // 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
- }
- 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)
- }
- }
+ type bundleInfo struct {
+ entry os.DirEntry
+ major int
+ minor int
+ patch int
+ arch string
+ }
+
+ groups := map[string][]bundleInfo{}
+ for _, file := range bundleFiles {
+ info, ok := parseBundleInfo(file.Name())
+ if !ok {
+ logging.Warnf("Skipping bundle with unrecognized version format: %s", file.Name())
+ continue
+ }
+ info.entry = file
+ key := fmt.Sprintf("%d.%d/%s", info.major, info.minor, info.arch)
+ groups[key] = append(groups[key], info)
+ }
+
+ for _, group := range groups {
+ sort.Slice(group, func(i, j int) bool {
+ return group[i].patch > group[j].patch
+ })
+ for i := keep; i < len(group); i++ {
+ file := group[i].entry
+ 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)
+ }
+ }
+ }
return nil
}
+
+func parseBundleInfo(name string) (bundleInfo, bool) {
+ base := strings.TrimSuffix(name, ".crcbundle")
+ parts := strings.Split(base, "_")
+ if len(parts) < 3 {
+ return bundleInfo{}, false
+ }
+ versionStr := parts[len(parts)-2]
+ arch := parts[len(parts)-1]
+ verParts := strings.Split(versionStr, ".")
+ if len(verParts) != 3 {
+ return bundleInfo{}, false
+ }
+ major, err1 := strconv.Atoi(verParts[0])
+ minor, err2 := strconv.Atoi(verParts[1])
+ patch, err3 := strconv.Atoi(verParts[2])
+ if err1 != nil || err2 != nil || err3 != nil {
+ return bundleInfo{}, false
+ }
+ return bundleInfo{major: major, minor: minor, patch: patch, arch: arch}, true
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(bundleFiles) <= keep { | |
| logging.Infof("Nothing to prune (found %d bundles, keeping %d)", len(bundleFiles), keep) | |
| return nil | |
| } | |
| // 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 | |
| } | |
| 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) | |
| } | |
| } | |
| if len(bundleFiles) <= keep { | |
| logging.Infof("Nothing to prune (found %d bundles, keeping %d)", len(bundleFiles), keep) | |
| return nil | |
| } | |
| type bundleInfo struct { | |
| entry os.DirEntry | |
| major int | |
| minor int | |
| patch int | |
| arch string | |
| } | |
| groups := map[string][]bundleInfo{} | |
| for _, file := range bundleFiles { | |
| info, ok := parseBundleInfo(file.Name()) | |
| if !ok { | |
| logging.Warnf("Skipping bundle with unrecognized version format: %s", file.Name()) | |
| continue | |
| } | |
| info.entry = file | |
| key := fmt.Sprintf("%d.%d/%s", info.major, info.minor, info.arch) | |
| groups[key] = append(groups[key], info) | |
| } | |
| for _, group := range groups { | |
| sort.Slice(group, func(i, j int) bool { | |
| return group[i].patch > group[j].patch | |
| }) | |
| for i := keep; i < len(group); i++ { | |
| file := group[i].entry | |
| 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) | |
| } | |
| } | |
| } | |
| return nil | |
| } | |
| func parseBundleInfo(name string) (bundleInfo, bool) { | |
| base := strings.TrimSuffix(name, ".crcbundle") | |
| parts := strings.Split(base, "_") | |
| if len(parts) < 3 { | |
| return bundleInfo{}, false | |
| } | |
| versionStr := parts[len(parts)-2] | |
| arch := parts[len(parts)-1] | |
| verParts := strings.Split(versionStr, ".") | |
| if len(verParts) != 3 { | |
| return bundleInfo{}, false | |
| } | |
| major, err1 := strconv.Atoi(verParts[0]) | |
| minor, err2 := strconv.Atoi(verParts[1]) | |
| patch, err3 := strconv.Atoi(verParts[2]) | |
| if err1 != nil || err2 != nil || err3 != nil { | |
| return bundleInfo{}, false | |
| } | |
| return bundleInfo{major: major, minor: minor, patch: patch, arch: arch}, true | |
| } |
🤖 Prompt for AI Agents
In `@cmd/crc/cmd/bundle/prune.go` around lines 45 - 68, The prune logic currently
sorts bundleFiles globally and removes everything beyond the newest keep, but
you must instead group bundles by major.minor (and arch/preset if applicable)
and prune within each group; update the code around bundleFiles, sort.Slice and
the removal loop to (1) parse each file.Name() (or bundle metadata via
file.Info()) to extract semver major.minor and arch, (2) build a
map[groupKey][]os.FileInfo (or []fs.File) where groupKey = major.minor + ":" +
arch, (3) for each group sort by semantic version (or ModTime if version not
parseable) to get newest-first, (4) keep the first keep entries per group and
call os.RemoveAll(filepath.Join(cacheDir, file.Name())) only for the rest, and
ensure errors from Info()/version parsing are handled (treat unparsable entries
as oldest so they get pruned last).
|
@albfan @anjannath @evidolob - Rebased this PR on the latest main branch. Currently monitoring CI checks - builds are passing, and the e2e-microshift tests have completed successfully. Waiting on remaining e2e/integration tests to complete. |
|
@sebrandon1: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Expanding on the already existing
crc bundlecommand with some exciting new functionality. I wanted to be able to download/manipulate the bundles available either locally or on the mirror website from the CRC client. I also wanted to be able to download bundles for different architectures if needed (will be used in quick-ocp more than likely).Example commands:
./crc bundle downloadthis is just the default command, and it'll download the bundle belonging to your crc release../crc bundle download 4.19 -vthis will download the latest available 4.19 bundle (similar to the way quick-ocp does it) where it will determine what the latest patch version is, and download it. You can also just supply an exact patch version that works as well. By default, it will check the downloaded bundle against the hash signature to make sure you have downloaded it correctly. Even thoughtcrcwon't be able to directly use the old bundles, its already been helpful for me to be able to download these bundles directly../crc bundle download 4.19 amd64 -vwill download a specific architecture version../crc bundle download listwill show you a list of all of the available versions of CRC/OCP available from the mirror website and whether or not you already have the bundle downloaded on your system already../crc bundle download clearwill clear all of your bundles out of your cache../crc bundle download prunewill clear all but the latest two patch versions belonging to a major version out of your cache. Like a watered down version ofclear.Real output:
Summary by CodeRabbit
New Features
Tests