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 crc-org member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain 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. |
WalkthroughThis PR introduces a new Changes
Sequence DiagramsequenceDiagram
participant CLI as crc CLI
participant Client as Machine Client
participant ImageLoadImpl as ImageLoad Logic
participant Registry as OpenShift Registry
participant Podman as Podman
participant OC as OC CLI
CLI->>Client: ImageLoad(config)
Client->>ImageLoadImpl: ImageLoad(config)
ImageLoadImpl->>ImageLoadImpl: Verify preset is OpenShift/OKD
ImageLoadImpl->>Registry: Check registry pod phase
ImageLoadImpl->>Registry: Verify defaultRoute enabled
ImageLoadImpl->>Registry: Patch defaultRoute if needed
ImageLoadImpl->>OC: Get kubeadmin password
ImageLoadImpl->>OC: oc login
ImageLoadImpl->>OC: oc whoami -t (get token)
ImageLoadImpl->>Podman: podman login with token
alt Source is TAR
ImageLoadImpl->>Podman: podman load -i <tar>
Podman-->>ImageLoadImpl: Return loaded image reference
else Source is Image Reference
ImageLoadImpl->>ImageLoadImpl: Use provided reference
end
ImageLoadImpl->>OC: oc image mirror to registry
OC-->>ImageLoadImpl: Success
ImageLoadImpl-->>Client: Success
Client-->>CLI: Success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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)
pkg/crc/machine/sync.go (1)
197-199: Consider adding state check for robustness.
ImageLoaddelegates directly without checkingcurrentState. If called while the cluster is stopping or deleting, it may fail in unexpected ways. Consider adding a state guard similar toprepareStopDeleteor at minimum document that callers must ensure the cluster is running.💡 Optional: Add state validation
func (s *Synchronized) ImageLoad(cfg types.ImageLoadConfig) error { + s.stateLock.Lock() + if s.currentStateUnlocked() != Idle { + s.stateLock.Unlock() + return errors.New("cluster is busy") + } + s.stateLock.Unlock() return s.underlying.ImageLoad(cfg) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/crc/machine/sync.go` around lines 197 - 199, The ImageLoad method on Synchronized currently forwards to s.underlying.ImageLoad without validating Synchronized.currentState; add a state guard similar to prepareStopDelete (or reuse that helper) to check that the cluster is in a valid running state before delegating, returning an appropriate error if the state is stopping/deleting, or alternatively document that callers must ensure the cluster is running; update the ImageLoad implementation in Synchronized to call the state-check helper (or inline the same checks) before invoking s.underlying.ImageLoad.cmd/crc/cmd/image/load.go (1)
83-91: Digest references (@sha256:...) are not handled correctly.If the source is a digest reference like
docker.io/library/nginx@sha256:abc123, the derived name becomesnginx@sha256:abc123which isn't a valid destinationname:tagformat. The colon check incorrectly passes due to the colon insha256:.♻️ Suggested fix to handle digests
// For image references, extract the name:tag portion ref := source if idx := strings.LastIndex(ref, "/"); idx >= 0 { ref = ref[idx+1:] } + // Strip digest if present (image@sha256:...) + if idx := strings.Index(ref, "@"); idx >= 0 { + ref = ref[:idx] + } if !strings.Contains(ref, ":") { ref += ":latest" } return ref🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/crc/cmd/image/load.go` around lines 83 - 91, The current logic using ref, strings.LastIndex and strings.Contains fails for digest references like "nginx@sha256:..."; update the handling so you first strip any digest portion (detect "@" and split at it) before extracting the name after the last "/" and before deciding whether to append ":latest" (i.e., check for ":" only on the name part after removing the digest). Adjust the code paths that set ref so that digest-based inputs produce a name:tag (e.g., "nginx:latest") rather than retaining the "@sha256:..." suffix.pkg/crc/machine/image_load.go (1)
49-63: Consider handling the case when no registry pods exist.If no pods match the selector,
items[0]returns an empty string, resulting in an unclear error message: "OpenShift image registry is not running (status: )". Consider using a jsonpath that handles this more gracefully or checking if any pods were found first.♻️ Suggested improvement
func checkRegistryRunning(ocConfig oc.Config) error { stdout, stderr, err := ocConfig.RunOcCommand( "get", "pods", "-n", "openshift-image-registry", "-l", "docker-registry=default", - "-o", "jsonpath={.items[0].status.phase}", + "-o", "jsonpath={.items[*].status.phase}", ) if err != nil { return fmt.Errorf("failed to check image registry status: %s: %w", stderr, err) } - if strings.TrimSpace(stdout) != "Running" { - return fmt.Errorf("OpenShift image registry is not running (status: %s)", strings.TrimSpace(stdout)) + status := strings.TrimSpace(stdout) + if status == "" { + return fmt.Errorf("OpenShift image registry pods not found") + } + if !strings.Contains(status, "Running") { + return fmt.Errorf("OpenShift image registry is not running (status: %s)", status) } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/crc/machine/image_load.go` around lines 49 - 63, checkRegistryRunning currently assumes items[0] exists and returns an unclear error when no pods match; update the logic in checkRegistryRunning to explicitly detect the "no pods" case by examining the raw stdout from ocConfig.RunOcCommand (or by first querying pod count/names with a jsonpath like "{.items[*].metadata.name}") and if stdout is empty/zero return a clear error such as "no image registry pods found" instead of the blank status, otherwise proceed to check the pod phase via strings.TrimSpace(stdout) and return the existing "not running" error when appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/crc/cmd/image/load.go`:
- Around line 64-69: isTarFile currently returns false when os.Stat fails, which
causes non-existent tar paths to be treated as image references; update the
logic so isTarFile only checks the filename extension (e.g., ".tar", ".tar.gz",
".tgz") and does not call os.Stat, and then in runLoad (where source is handled)
explicitly detect "tar-like" paths and validate existence with os.Stat,
returning a clear error like "tar file not found: <source>" if missing;
alternatively, modify isTarFile to isTarLikePath (extension-only) and add the
existence check in runLoad before treating the source as an image reference.
---
Nitpick comments:
In `@cmd/crc/cmd/image/load.go`:
- Around line 83-91: The current logic using ref, strings.LastIndex and
strings.Contains fails for digest references like "nginx@sha256:..."; update the
handling so you first strip any digest portion (detect "@" and split at it)
before extracting the name after the last "/" and before deciding whether to
append ":latest" (i.e., check for ":" only on the name part after removing the
digest). Adjust the code paths that set ref so that digest-based inputs produce
a name:tag (e.g., "nginx:latest") rather than retaining the "@sha256:..."
suffix.
In `@pkg/crc/machine/image_load.go`:
- Around line 49-63: checkRegistryRunning currently assumes items[0] exists and
returns an unclear error when no pods match; update the logic in
checkRegistryRunning to explicitly detect the "no pods" case by examining the
raw stdout from ocConfig.RunOcCommand (or by first querying pod count/names with
a jsonpath like "{.items[*].metadata.name}") and if stdout is empty/zero return
a clear error such as "no image registry pods found" instead of the blank
status, otherwise proceed to check the pod phase via strings.TrimSpace(stdout)
and return the existing "not running" error when appropriate.
In `@pkg/crc/machine/sync.go`:
- Around line 197-199: The ImageLoad method on Synchronized currently forwards
to s.underlying.ImageLoad without validating Synchronized.currentState; add a
state guard similar to prepareStopDelete (or reuse that helper) to check that
the cluster is in a valid running state before delegating, returning an
appropriate error if the state is stopping/deleting, or alternatively document
that callers must ensure the cluster is running; update the ImageLoad
implementation in Synchronized to call the state-check helper (or inline the
same checks) before invoking s.underlying.ImageLoad.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6bbc26b-6260-494f-8da0-d7178dc66e56
📒 Files selected for processing (10)
cmd/crc/cmd/image/image.gocmd/crc/cmd/image/load.gocmd/crc/cmd/root.gocmd/crc/cmd/root_test.gopkg/crc/machine/client.gopkg/crc/machine/fakemachine/client.gopkg/crc/machine/image_load.gopkg/crc/machine/sync.gopkg/crc/machine/sync_test.gopkg/crc/machine/types/types.go
| func isTarFile(source string) bool { | ||
| if _, err := os.Stat(source); err != nil { | ||
| return false | ||
| } | ||
| return strings.HasSuffix(source, ".tar") || strings.HasSuffix(source, ".tar.gz") || strings.HasSuffix(source, ".tgz") | ||
| } |
There was a problem hiding this comment.
Non-existent tar files silently fall through to image reference handling.
If a user specifies a tar file path that doesn't exist (e.g., typo in myapp.tar), os.Stat fails and this returns false. The code then treats it as an image reference, leading to a confusing error about failing to mirror from a non-existent registry. Consider checking separately: validate file existence for paths with tar extensions and return a clear error if the file is missing.
🛠️ Suggested improvement
func isTarFile(source string) bool {
- if _, err := os.Stat(source); err != nil {
- return false
- }
- return strings.HasSuffix(source, ".tar") || strings.HasSuffix(source, ".tar.gz") || strings.HasSuffix(source, ".tgz")
+ looksLikeTar := strings.HasSuffix(source, ".tar") || strings.HasSuffix(source, ".tar.gz") || strings.HasSuffix(source, ".tgz")
+ if !looksLikeTar {
+ return false
+ }
+ _, err := os.Stat(source)
+ return err == nil
}Then in runLoad, you could add explicit validation:
if isTarLikePath(source) {
if _, err := os.Stat(source); err != nil {
return fmt.Errorf("tar file not found: %s", source)
}
}📝 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 isTarFile(source string) bool { | |
| if _, err := os.Stat(source); err != nil { | |
| return false | |
| } | |
| return strings.HasSuffix(source, ".tar") || strings.HasSuffix(source, ".tar.gz") || strings.HasSuffix(source, ".tgz") | |
| } | |
| func isTarFile(source string) bool { | |
| looksLikeTar := strings.HasSuffix(source, ".tar") || strings.HasSuffix(source, ".tar.gz") || strings.HasSuffix(source, ".tgz") | |
| if !looksLikeTar { | |
| return false | |
| } | |
| _, err := os.Stat(source) | |
| return err == nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/crc/cmd/image/load.go` around lines 64 - 69, isTarFile currently returns
false when os.Stat fails, which causes non-existent tar paths to be treated as
image references; update the logic so isTarFile only checks the filename
extension (e.g., ".tar", ".tar.gz", ".tgz") and does not call os.Stat, and then
in runLoad (where source is handled) explicitly detect "tar-like" paths and
validate existence with os.Stat, returning a clear error like "tar file not
found: <source>" if missing; alternatively, modify isTarFile to isTarLikePath
(extension-only) and add the existence check in runLoad before treating the
source as an image reference.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/crc/cmd/ssh.go`:
- Around line 53-72: The code uses Unix-only behavior: replace the literal
"/dev/null" with os.DevNull in the sshArgs construction to be cross-platform and
either add a build tag "//go:build !windows" at the top of this file to restrict
it to Unix, or implement a Windows-safe fallback for the no-args branch by using
exec.Command(sshPath, sshArgs[1:]...) (same pattern used in the len(args)>0
branch) instead of syscall.Exec; update references around sshArgs, exec.Command,
and syscall.Exec accordingly so Windows builds use the subprocess path while
Unix can keep syscall.Exec or be separated via the build constraint.
In `@cmd/crc/cmd/vm/stats.go`:
- Around line 195-200: Replace interactive sudo invocations with non-interactive
ones in the stats command list: change all occurrences of the string patterns
like "sudo crictl pods -q", "sudo crictl ps -q", "sudo crictl images -q" and
"sudo crictl stats -o json" to use "sudo -n crictl …" so the code that
builds/executes the command list (the slice containing `echo "===TOP==="` and
the crictl lines) fails immediately instead of hanging if sudo asks for a
password.
- Around line 131-147: After verifying existence with client.Exists(), add a
running-state check by calling client.IsRunning(); if IsRunning returns an error
propagate it, and if it returns false return crcErr.VMNotRunning so we don't
attempt to fetch connection details or open SSH to a stopped VM. Place this
check between the existing Exists() block and the call to
client.ConnectionDetails()/ssh.CreateRunner() (referencing client.IsRunning,
client.ConnectionDetails, ssh.CreateRunner, and crcErr.VMNotRunning).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf5f0a5d-66a2-40aa-a726-0e3e8522dae9
📒 Files selected for processing (5)
cmd/crc/cmd/root.gocmd/crc/cmd/root_test.gocmd/crc/cmd/ssh.gocmd/crc/cmd/vm/stats.gocmd/crc/cmd/vm/vm.go
✅ Files skipped from review due to trivial changes (1)
- cmd/crc/cmd/root_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/crc/cmd/root.go
cmd/crc/cmd/ssh.go
Outdated
| sshArgs := []string{ | ||
| "ssh", | ||
| "-o", "StrictHostKeyChecking=no", | ||
| "-o", "UserKnownHostsFile=/dev/null", | ||
| "-o", "LogLevel=ERROR", | ||
| "-i", sshKey, | ||
| "-p", strconv.Itoa(connectionDetails.SSHPort), | ||
| fmt.Sprintf("%s@%s", connectionDetails.SSHUsername, connectionDetails.IP), | ||
| } | ||
|
|
||
| if len(args) > 0 { | ||
| sshArgs = append(sshArgs, args...) | ||
| cmd := exec.Command(sshPath, sshArgs[1:]...) | ||
| cmd.Stdin = os.Stdin | ||
| cmd.Stdout = os.Stdout | ||
| cmd.Stderr = os.Stderr | ||
| return cmd.Run() | ||
| } | ||
|
|
||
| return syscall.Exec(sshPath, sshArgs, os.Environ()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for existing build tags or platform-specific patterns in the codebase
echo "=== Checking for build tags in cmd/crc/cmd/ ==="
rg -l '//go:build|// \+build' cmd/crc/cmd/ || echo "No build tags found"
echo ""
echo "=== Checking for syscall.Exec usage elsewhere ==="
rg -n 'syscall\.Exec' --type go
echo ""
echo "=== Checking for /dev/null usage ==="
rg -n '/dev/null' --type go -C2
echo ""
echo "=== Checking for os.DevNull usage ==="
rg -n 'os\.DevNull' --type goRepository: crc-org/crc
Length of output: 24724
🏁 Script executed:
# Check go.mod for platform constraints
head -20 go.mod
# Search for Windows-related build constraints in the codebase
echo "=== Searching for Windows build constraints ==="
rg '//go:build.*windows|// \+build.*windows' --type go
# Check if there are any other Unix-specific calls restricted by build tags
echo ""
echo "=== Files with build tags (excluding vendor) ==="
rg -l '//go:build|// \+build' --type go cmd/ pkg/ 2>/dev/null | grep -v vendor
# Check README or similar documentation for platform support
echo ""
echo "=== Checking for platform info in documentation ==="
find . -maxdepth 2 -type f \( -name "README*" -o -name "PLATFORMS*" -o -name "*.md" \) ! -path "./vendor/*" -exec head -50 {} + 2>/dev/null | head -100Repository: crc-org/crc
Length of output: 38264
🏁 Script executed:
# Get the exact context around the problematic code
cat -n cmd/crc/cmd/ssh.go | sed -n '50,75p'
# Also check if there are any existing Windows-specific implementations or alternatives
rg -A5 -B5 'syscall\.Exec' cmd/crc/cmd/ssh.goRepository: crc-org/crc
Length of output: 928
Add build constraint or implement cross-platform SSH execution.
This code uses Unix-specific features that fail on Windows despite being compiled on all platforms:
- Line 56:
/dev/nulldoesn't exist on Windows—useos.DevNullinstead. - Line 72:
syscall.Execdoesn't replace the process on Windows like it does on Unix.
Since Windows is a supported platform (evidenced by Windows CI and dependencies), consider either:
- Adding
//go:build !windowsto restrict this file to Unix platforms, or - Modifying the no-args branch (line 72) to use the subprocess approach (lines 63-69) on Windows via a conditional build or runtime check.
🛠️ Fix for /dev/null cross-platform compatibility
sshArgs := []string{
"ssh",
"-o", "StrictHostKeyChecking=no",
- "-o", "UserKnownHostsFile=/dev/null",
+ "-o", fmt.Sprintf("UserKnownHostsFile=%s", os.DevNull),
"-o", "LogLevel=ERROR",📝 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.
| sshArgs := []string{ | |
| "ssh", | |
| "-o", "StrictHostKeyChecking=no", | |
| "-o", "UserKnownHostsFile=/dev/null", | |
| "-o", "LogLevel=ERROR", | |
| "-i", sshKey, | |
| "-p", strconv.Itoa(connectionDetails.SSHPort), | |
| fmt.Sprintf("%s@%s", connectionDetails.SSHUsername, connectionDetails.IP), | |
| } | |
| if len(args) > 0 { | |
| sshArgs = append(sshArgs, args...) | |
| cmd := exec.Command(sshPath, sshArgs[1:]...) | |
| cmd.Stdin = os.Stdin | |
| cmd.Stdout = os.Stdout | |
| cmd.Stderr = os.Stderr | |
| return cmd.Run() | |
| } | |
| return syscall.Exec(sshPath, sshArgs, os.Environ()) | |
| sshArgs := []string{ | |
| "ssh", | |
| "-o", "StrictHostKeyChecking=no", | |
| "-o", fmt.Sprintf("UserKnownHostsFile=%s", os.DevNull), | |
| "-o", "LogLevel=ERROR", | |
| "-i", sshKey, | |
| "-p", strconv.Itoa(connectionDetails.SSHPort), | |
| fmt.Sprintf("%s@%s", connectionDetails.SSHUsername, connectionDetails.IP), | |
| } | |
| if len(args) > 0 { | |
| sshArgs = append(sshArgs, args...) | |
| cmd := exec.Command(sshPath, sshArgs[1:]...) | |
| cmd.Stdin = os.Stdin | |
| cmd.Stdout = os.Stdout | |
| cmd.Stderr = os.Stderr | |
| return cmd.Run() | |
| } | |
| return syscall.Exec(sshPath, sshArgs, os.Environ()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/crc/cmd/ssh.go` around lines 53 - 72, The code uses Unix-only behavior:
replace the literal "/dev/null" with os.DevNull in the sshArgs construction to
be cross-platform and either add a build tag "//go:build !windows" at the top of
this file to restrict it to Unix, or implement a Windows-safe fallback for the
no-args branch by using exec.Command(sshPath, sshArgs[1:]...) (same pattern used
in the len(args)>0 branch) instead of syscall.Exec; update references around
sshArgs, exec.Command, and syscall.Exec accordingly so Windows builds use the
subprocess path while Unix can keep syscall.Exec or be separated via the build
constraint.
cmd/crc/cmd/vm/stats.go
Outdated
| exists, err := client.Exists() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if !exists { | ||
| return crcErr.VMNotExist | ||
| } | ||
|
|
||
| details, err := client.ConnectionDetails() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| runner, err := ssh.CreateRunner(details.IP, details.SSHPort, details.SSHKeys...) | ||
| if err != nil { | ||
| return fmt.Errorf("cannot create SSH connection: %w", err) | ||
| } |
There was a problem hiding this comment.
Check VM running state before opening SSH session.
On Line 131, only existence is validated. If the VM exists but is stopped, this path falls through to connection errors instead of a clear “not running” message. Add an IsRunning() check after existence validation and return the dedicated not-running error.
Proposed fix
exists, err := client.Exists()
if err != nil {
return err
}
if !exists {
return crcErr.VMNotExist
}
+
+ running, err := client.IsRunning()
+ if err != nil {
+ return err
+ }
+ if !running {
+ return crcErr.VMNotRunning
+ }
details, err := client.ConnectionDetails()
if err != nil {
return err
}📝 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.
| exists, err := client.Exists() | |
| if err != nil { | |
| return err | |
| } | |
| if !exists { | |
| return crcErr.VMNotExist | |
| } | |
| details, err := client.ConnectionDetails() | |
| if err != nil { | |
| return err | |
| } | |
| runner, err := ssh.CreateRunner(details.IP, details.SSHPort, details.SSHKeys...) | |
| if err != nil { | |
| return fmt.Errorf("cannot create SSH connection: %w", err) | |
| } | |
| exists, err := client.Exists() | |
| if err != nil { | |
| return err | |
| } | |
| if !exists { | |
| return crcErr.VMNotExist | |
| } | |
| running, err := client.IsRunning() | |
| if err != nil { | |
| return err | |
| } | |
| if !running { | |
| return crcErr.VMNotRunning | |
| } | |
| details, err := client.ConnectionDetails() | |
| if err != nil { | |
| return err | |
| } | |
| runner, err := ssh.CreateRunner(details.IP, details.SSHPort, details.SSHKeys...) | |
| if err != nil { | |
| return fmt.Errorf("cannot create SSH connection: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/crc/cmd/vm/stats.go` around lines 131 - 147, After verifying existence
with client.Exists(), add a running-state check by calling client.IsRunning();
if IsRunning returns an error propagate it, and if it returns false return
crcErr.VMNotRunning so we don't attempt to fetch connection details or open SSH
to a stopped VM. Place this check between the existing Exists() block and the
call to client.ConnectionDetails()/ssh.CreateRunner() (referencing
client.IsRunning, client.ConnectionDetails, ssh.CreateRunner, and
crcErr.VMNotRunning).
cmd/crc/cmd/vm/stats.go
Outdated
| `sudo crictl pods -q 2>/dev/null | wc -l`, | ||
| `sudo crictl ps -q 2>/dev/null | wc -l`, | ||
| `sudo crictl images -q 2>/dev/null | wc -l`, | ||
| // Top containers by memory (raw JSON, parsed in Go) | ||
| `echo "===TOP==="`, | ||
| `sudo crictl stats -o json 2>/dev/null`, |
There was a problem hiding this comment.
Make sudo crictl calls non-interactive to prevent command hangs.
On Line 195–200, sudo is invoked without -n. If sudo ever requires auth, this can block the stats command. Use sudo -n for all crictl calls so failures are immediate and non-interactive.
Proposed fix
- `sudo crictl pods -q 2>/dev/null | wc -l`,
- `sudo crictl ps -q 2>/dev/null | wc -l`,
- `sudo crictl images -q 2>/dev/null | wc -l`,
+ `sudo -n crictl pods -q 2>/dev/null | wc -l`,
+ `sudo -n crictl ps -q 2>/dev/null | wc -l`,
+ `sudo -n crictl images -q 2>/dev/null | wc -l`,
// Top containers by memory (raw JSON, parsed in Go)
`echo "===TOP==="`,
- `sudo crictl stats -o json 2>/dev/null`,
+ `sudo -n crictl stats -o json 2>/dev/null`,📝 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.
| `sudo crictl pods -q 2>/dev/null | wc -l`, | |
| `sudo crictl ps -q 2>/dev/null | wc -l`, | |
| `sudo crictl images -q 2>/dev/null | wc -l`, | |
| // Top containers by memory (raw JSON, parsed in Go) | |
| `echo "===TOP==="`, | |
| `sudo crictl stats -o json 2>/dev/null`, | |
| `sudo -n crictl pods -q 2>/dev/null | wc -l`, | |
| `sudo -n crictl ps -q 2>/dev/null | wc -l`, | |
| `sudo -n crictl images -q 2>/dev/null | wc -l`, | |
| // Top containers by memory (raw JSON, parsed in Go) | |
| `echo "===TOP==="`, | |
| `sudo -n crictl stats -o json 2>/dev/null`, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/crc/cmd/vm/stats.go` around lines 195 - 200, Replace interactive sudo
invocations with non-interactive ones in the stats command list: change all
occurrences of the string patterns like "sudo crictl pods -q", "sudo crictl ps
-q", "sudo crictl images -q" and "sudo crictl stats -o json" to use "sudo -n
crictl …" so the code that builds/executes the command list (the slice
containing `echo "===TOP==="` and the crictl lines) fails immediately instead of
hanging if sudo asks for a password.
sebrandon1
force-pushed
the
feat/image-load-command
branch
from
b33e3ea to
60dc3f2
Compare
|
@sebrandon1 what is the purpose of this command, I looked at the implementation and it is basically same what we ask user to do so they can learn around how to push image to the openshift cluster. |
|
@praveenkumar Thanks for the review! I was just trying to improve the tool by building in the feature right into I thought it was pretty useful! |
Add crc image load to push container images into the CRC cluster's internal OpenShift registry. Supports tar files and registry references. Automatically checks registry health, enables the default route if needed, and handles OAuth authentication via kubeadmin credentials.
sebrandon1
force-pushed
the
feat/image-load-command
branch
from
60dc3f2 to
11ecd92
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/crc/cmd/image/load.go (1)
46-52:⚠️ Potential issue | 🟡 MinorDon’t use
os.Statto decide whether the source is a tar archive.Line 65 returns
falseformissing.tar, sorunLoadfalls through to the registry-reference path and users get a mirror/pull error instead oftar file not found. Check the suffix first, then validate tar-like paths separately inrunLoad.Also applies to: 64-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/crc/cmd/image/load.go` around lines 46 - 52, The code currently relies on isTarFile (and os.Stat) to decide whether source is a tar and misclassifies missing paths; update isTarFile to detect tar archives by suffix (e.g., .tar, .tar.gz, .tgz, .oci, .tar.xz) using string checks (strings.HasSuffix) rather than os.Stat, and then in runLoad after you determine tar-like by suffix, separately validate the path exists (using os.Stat) and return a clear "tar file not found" error if missing; adjust the flow around deriveImageName, isTarFile, and the registry-reference branch so suffix detection happens first and existence check happens only for tar paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/crc/cmd/image/load.go`:
- Around line 46-52: The code currently relies on isTarFile (and os.Stat) to
decide whether source is a tar and misclassifies missing paths; update isTarFile
to detect tar archives by suffix (e.g., .tar, .tar.gz, .tgz, .oci, .tar.xz)
using string checks (strings.HasSuffix) rather than os.Stat, and then in runLoad
after you determine tar-like by suffix, separately validate the path exists
(using os.Stat) and return a clear "tar file not found" error if missing; adjust
the flow around deriveImageName, isTarFile, and the registry-reference branch so
suffix detection happens first and existence check happens only for tar paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c721372-c3a7-4928-a876-42d0cbddb219
📒 Files selected for processing (10)
cmd/crc/cmd/image/image.gocmd/crc/cmd/image/load.gocmd/crc/cmd/root.gocmd/crc/cmd/root_test.gopkg/crc/machine/client.gopkg/crc/machine/fakemachine/client.gopkg/crc/machine/image_load.gopkg/crc/machine/sync.gopkg/crc/machine/sync_test.gopkg/crc/machine/types/types.go
✅ Files skipped from review due to trivial changes (3)
- pkg/crc/machine/types/types.go
- pkg/crc/machine/client.go
- cmd/crc/cmd/root_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/crc/machine/fakemachine/client.go
- cmd/crc/cmd/root.go
- pkg/crc/machine/sync_test.go
- cmd/crc/cmd/image/image.go
- pkg/crc/machine/sync.go
| // For image references, extract the name:tag portion | ||
| ref := source | ||
| if idx := strings.LastIndex(ref, "/"); idx >= 0 { | ||
| ref = ref[idx+1:] | ||
| } | ||
| if !strings.Contains(ref, ":") { | ||
| ref += ":latest" | ||
| } |
There was a problem hiding this comment.
Handle digest-pinned references before the tag check.
Line 88 treats nginx@sha256:... as “already tagged” because digests also contain :, so the derived default becomes nginx@sha256:... instead of a name:tag. Strip @... before checking for a tag, or require --name for digest sources.
🛠️ Proposed fix
ref := source
if idx := strings.LastIndex(ref, "/"); idx >= 0 {
ref = ref[idx+1:]
}
+ if idx := strings.Index(ref, "@"); idx >= 0 {
+ ref = ref[:idx]
+ }
if !strings.Contains(ref, ":") {
ref += ":latest"
}
return ref📝 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.
| // For image references, extract the name:tag portion | |
| ref := source | |
| if idx := strings.LastIndex(ref, "/"); idx >= 0 { | |
| ref = ref[idx+1:] | |
| } | |
| if !strings.Contains(ref, ":") { | |
| ref += ":latest" | |
| } | |
| // For image references, extract the name:tag portion | |
| ref := source | |
| if idx := strings.LastIndex(ref, "/"); idx >= 0 { | |
| ref = ref[idx+1:] | |
| } | |
| if idx := strings.Index(ref, "@"); idx >= 0 { | |
| ref = ref[:idx] | |
| } | |
| if !strings.Contains(ref, ":") { | |
| ref += ":latest" | |
| } |
2 participants
Summary
crc image loadcommand to push container images into the CRC cluster's internal OpenShift registrycrc image load myapp.tar) and registry references (crc image load nginx:latest)Usage
Test plan
make lintpassesgo test ./cmd/crc/cmd/... ./pkg/crc/machine/...)--namespaceand--nameflagsoc get isSummary by CodeRabbit
Release Notes
image loadcommand supports importing container images from tar archives or image references