PoC: Add bundle download cmd #5037
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 bundle management subcommands (download, list, clear, prune), supporting utilities for version discovery and resolution, cache inspection/pruning, mirror URL configuration, and test updates for new manpages. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✨ 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 |
Address comments 1 Address comments; consts, split bundle commands into files
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.
| func runList(args []string, config *crcConfig.Config) error { | ||
| preset := crcConfig.GetPreset(config) | ||
| versions, err := fetchAvailableVersions(preset) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if len(versions) == 0 { | ||
| logging.Infof("No bundles found for preset %s", preset) | ||
| return nil | ||
| } | ||
|
|
||
| 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 | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add validation for the number of arguments.
The function checks len(args) > 0 to determine if a filter is provided, but doesn't validate that at most one argument is supplied. If a user provides multiple arguments (e.g., crc bundle list 4.19 4.20), only the first is used and the rest are silently ignored, which may confuse users.
🔎 Apply this diff to add argument validation:
func runList(args []string, config *crcConfig.Config) error {
+ if len(args) > 1 {
+ return fmt.Errorf("too many arguments: expected at most 1 version filter, got %d", len(args))
+ }
+
preset := crcConfig.GetPreset(config)
versions, err := fetchAvailableVersions(preset)📝 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.
| func runList(args []string, config *crcConfig.Config) error { | |
| preset := crcConfig.GetPreset(config) | |
| versions, err := fetchAvailableVersions(preset) | |
| if err != nil { | |
| return err | |
| } | |
| if len(versions) == 0 { | |
| logging.Infof("No bundles found for preset %s", preset) | |
| return nil | |
| } | |
| 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 | |
| } | |
| } | |
| } | |
| func runList(args []string, config *crcConfig.Config) error { | |
| if len(args) > 1 { | |
| return fmt.Errorf("too many arguments: expected at most 1 version filter, got %d", len(args)) | |
| } | |
| preset := crcConfig.GetPreset(config) | |
| versions, err := fetchAvailableVersions(preset) | |
| if err != nil { | |
| return err | |
| } | |
| if len(versions) == 0 { | |
| logging.Infof("No bundles found for preset %s", preset) | |
| return nil | |
| } | |
| 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 | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In cmd/crc/cmd/bundle/list.go around lines 23 to 47, the handler accepts a
filter if len(args) > 0 but does not validate that the caller provided at most
one argument, causing extra args to be silently ignored; add validation that
returns a clear error (or usage message) when len(args) > 1 so the command fails
fast and informs the user that only a single version filter is accepted,
otherwise proceed to parse args[0] as currently implemented.
|
@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
✏️ Tip: You can customize this high-level summary in your review settings.