Support deploying locally built images to local cluster#204
Open
mclasmeier wants to merge 17 commits into
Open
Support deploying locally built images to local cluster#204mclasmeier wants to merge 17 commits into
mclasmeier wants to merge 17 commits into
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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: 5
🧹 Nitpick comments (7)
internal/deployer/local_images.go (2)
57-68: ⚡ Quick winConsider tracking and reporting transfer success/failure counts.
The transfer loop logs individual failures but doesn't provide a summary. For better user experience, consider counting successful and failed transfers and logging a summary after the loop completes.
Example:
transferred, failed := 0, 0 for _, image := range localImages { // ... existing logic ... if err := preLoader.SendImage(ctx, image); err != nil { failed++ d.logger.Warningf("...") } else { transferred++ } } if transferred > 0 || failed > 0 { d.logger.Infof("Image transfer complete: %d succeeded, %d failed", transferred, failed) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/deployer/local_images.go` around lines 57 - 68, Add counters to the image transfer loop to track successes and failures: inside the loop over localImages (where you currently check slices.Contains(availableImagesInCluster, image) and call preLoader.SendImage(ctx, image)), increment a success counter when SendImage returns nil and a failure counter when it returns an error (you already log per-image failures via d.logger.Warningf referencing d.config.Roxie.ClusterType); after the loop log a summary via d.logger.Infof reporting the transferred (success) and failed counts so users get an overall result of the transfer operation.
27-35: 💤 Low valueK3s and CRC cluster types not handled despite IsLocal() returning true.
ClusterType.IsLocal()returnstruefor K3s and CRC (cluster_type.go:80), butGetPreLoaderForCluster()only handles Kind and Minikube. This means K3s and CRC will returnErrLocalImagesUnsupported, which may be intentional if preloaders aren't implemented yet.If K3s/CRC support is planned, consider adding TODO comments. Otherwise, this is correct behavior for unsupported-but-local cluster types.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/deployer/local_images.go` around lines 27 - 35, GetPreLoaderForCluster currently only handles ClusterTypeKind and ClusterTypeMinikube while IsLocal() can be true for K3s and CRC; update GetPreLoaderForCluster to explicitly account for K3s and CRC by either (a) adding explicit case branches for types.ClusterTypeK3s and types.ClusterTypeCRC that return a clear ErrLocalImagesUnsupported with a TODO comment referencing newK3sImagePreloader/newCRCImagePreloader (or planned implementations), or (b) implement and return concrete preloaders via newK3sImagePreloader/newCRCImagePreloader if support exists; ensure the branches and TODOs reference the GetPreLoaderForCluster function and the cluster type symbols so future maintainers know these local cluster types are intentionally unsupported or where to add implementations.internal/ocihelper/ocihelper.go (1)
75-93: 💤 Low valuePlatform inconsistency between local and remote image loading.
The local daemon call (line 75) doesn't specify a platform, while the remote pull (lines 85-93) explicitly requests
linux/amd64. If a local daemon contains an operator bundle image for a different platform (e.g.,linux/arm64), the function will return that platform variant when loading locally, but would fetchlinux/amd64when pulling remotely.For operator bundles containing platform-agnostic YAML manifests, this difference is likely acceptable. However, consider whether explicitly specifying the platform for both code paths would provide more consistent behavior, or document this intentional difference.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/ocihelper/ocihelper.go` around lines 75 - 93, The local image load uses daemon.Image(ref, daemon.WithContext(ctx)) while the remote pull uses remote.Image(..., remote.WithPlatform(platform)), causing platform mismatch; update the local path to request the same platform by passing the same platform variable (e.g. use daemon.WithPlatform(platform) or the equivalent option accepted by daemon.Image) and ensure the v1.Platform{OS:"linux",Architecture:"amd64"} is defined before both calls so both daemon.Image and remote.Image use the identical platform selection.internal/deployer/local_images_kind.go (2)
31-33: 💤 Low valueClarify the nil return when container runtime socket is unavailable.
Returning
(nil, nil)whencontainerRuntimeSocketis empty could be ambiguous. This currently signals "no images found, no error," which allows the preloader to proceed without listing local images.Consider whether returning
ErrLocalImageRetrievalNotSupportedwould be more explicit, similar tocustomImagePreLoader.GetImages(). However, if the current behavior (silently skipping local image detection) is intentional, the implementation is acceptable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/deployer/local_images_kind.go` around lines 31 - 33, The current early return in the method that checks k.containerRuntimeSocket returns (nil, nil), which is ambiguous; change it to return a sentinel error (e.g., ErrLocalImageRetrievalNotSupported) instead of (nil, nil) so callers can distinguish “unsupported” from “no images” — update the function in internal/deployer/local_images_kind.go that references k.containerRuntimeSocket to return the sentinel error (matching the behavior of customImagePreLoader.GetImages()) and add/update the sentinel error symbol if it doesn't exist.
35-39: ⚖️ Poor tradeoffConsider multi-node Kind cluster support.
The implementation hardcodes the control-plane node name (line 35) and only retrieves images from that node. In multi-node Kind clusters, images may be distributed across worker nodes as well. This could result in incomplete image lists.
If multi-node Kind cluster support is planned, consider:
- Listing all cluster nodes
- Querying images from each node
- Deduplicating the combined results
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/deployer/local_images_kind.go` around lines 35 - 39, The current code only inspects the single hardcoded nodeName (k.kindClusterName + "-control-plane") which misses images on additional nodes; update the logic in internal/deployer/local_images_kind.go to list all Kind node containers (e.g., enumerate containers matching k.kindClusterName prefix or query cluster nodes) and call containerrt.ExecInContainer with k.containerRuntimeSocket for each node instead of only the control-plane, collect the images from each response, deduplicate the combined results, and return the unified list; ensure errors from per-node ExecInContainer are handled/aggregated and still wrapped with context similar to the existing fmt.Errorf call.internal/deployer/local_images_custom.go (1)
28-39: 💤 Low valueUnderstood: Shell command execution is intentional but document security implications.
The static analysis tool correctly identifies the command injection pattern on line 32. However, this is the intended behavior for a custom image preloader where users explicitly provide shell commands via the
--image-preload-commandCLI flag.Consider adding documentation (inline or user-facing) warning that:
- The command is executed as-is via
/bin/sh -c- Users must ensure the command string is trusted and properly escaped
- The
IMAGEenvironment variable will be set to the image tag being transferredThe implementation itself is correct for its purpose.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/deployer/local_images_custom.go` around lines 28 - 39, Add an explicit security warning and usage note for the custom image preloader: document (near customImagePreLoader.SendImage and the CLI flag --image-preload-command) that the user-provided command is executed via "/bin/sh -c" as-is, that this can lead to command injection if untrusted input is supplied, that callers must only provide trusted and properly escaped commands, and that the IMAGE environment variable is set to the image tag for use inside the command; place this note in the nearest user-facing docs/help text and/or an inline comment next to SendImage and the flag registration.internal/deployer/local_images_minikube.go (1)
29-32: ⚡ Quick winUse
CombinedOutput()for better error diagnostics.Line 29 uses
Output(), which captures only stdout and discards stderr. When the command fails, valuable error messages from stderr are lost, making debugging more difficult.Replace
Output()withCombinedOutput()to capture both stdout and stderr. This matches the pattern used ingenericImageSender.SendImage()(line 36 inlocal_images_generic.go) and provides better error context.📊 Proposed fix to capture stderr
- output, err := exec.CommandContext(ctx, "minikube", "ssh", "--", "crictl", "images", "-o", "json").Output() + output, err := exec.CommandContext(ctx, "minikube", "ssh", "--", "crictl", "images", "-o", "json").CombinedOutput() if err != nil { return nil, fmt.Errorf("listing images in minikube node: %w", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/deployer/local_images_minikube.go` around lines 29 - 32, Replace the exec.CommandContext(...).Output() call in local_images_minikube.go (the block that runs "minikube ssh -- crictl images -o json") with CombinedOutput() so stderr is captured; when err != nil, include the combined output in the returned fmt.Errorf to provide full diagnostic information (mirror the pattern used in genericImageSender.SendImage() in local_images_generic.go).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/containerrt/containerrt.go`:
- Around line 94-132: The comment mentions k3s but socketCandidates() does not
include k3s paths; update socketCandidates() to add common k3s/containerd socket
locations (e.g. "/run/k3s/containerd/containerd.sock" and any other distro
variants you want to support) so ResolveSocket() can detect k3s runtimes; modify
the slice appended in socketCandidates() to include those k3s paths alongside
existing podman/docker entries and ensure returned candidates are checked by
ResolveSocket().
- Around line 87-90: The current loop that collects image tags into the local
variable tags from result.Images can append empty RepoTags; update the loop in
the containerrt.go function so it iterates inner RepoTags and only appends
non-empty strings (filter tag == ""), mirroring the behavior in ListLocalImages;
reference the variables result.Images, img.RepoTags and the tags slice when
making this change.
- Around line 31-34: The current loop in containerrt.go appends img.RepoTags
directly into tags, which can introduce empty strings or nil entries; update the
logic that builds tags (the variables images, img, and tags are involved) to
iterate each img.RepoTags and only append non-empty tag values (e.g., check tag
!= "" or trim and check length) to ensure the returned slice contains only valid
tags.
In `@internal/deployer/acs_images.go`:
- Line 21: The image name for the operator bundle is inconsistent: the images
append uses "stackrox-operator-bundle" while OperatorBundleImage() returns
"release-operator-bundle" when KonfluxImages is enabled; pick the authoritative
naming and make both produce the same reference. Update the append call in the
images building code to use the same naming logic as OperatorBundleImage() (or
refactor both to call a single helper/constant), e.g., replace the hardcoded
"stackrox-operator-bundle" with the value produced by OperatorBundleImage() or
change OperatorBundleImage() to return "stackrox-operator-bundle" so both image
construction paths (the images append and the OperatorBundleImage function) are
aligned.
In `@internal/deployer/local_images.go`:
- Around line 75-88: The collectLocalImages function currently swallows errors
from containerrt.ListLocalImages by returning (nil, nil); update it to either
propagate the error or explicitly document/encode silent failure: Option A
(recommended) return the wrapped error from collectLocalImages (e.g., return
nil, fmt.Errorf("could not query container runtime at %s: %w", socket, err)) so
callers like TryTransferLocalImages receive and can handle the error; Option B
(if silent best-effort is intentional) change the signature of
collectLocalImages to return []string (remove error), add a comment explaining
"Best-effort local image collection; failures are non-fatal", and adjust callers
(TryTransferLocalImages) to handle the new signature and logging (use
d.logger.Dimf where appropriate). Ensure references to
containerrt.ListLocalImages and d.containerRuntimeSocket remain correct.
---
Nitpick comments:
In `@internal/deployer/local_images_custom.go`:
- Around line 28-39: Add an explicit security warning and usage note for the
custom image preloader: document (near customImagePreLoader.SendImage and the
CLI flag --image-preload-command) that the user-provided command is executed via
"/bin/sh -c" as-is, that this can lead to command injection if untrusted input
is supplied, that callers must only provide trusted and properly escaped
commands, and that the IMAGE environment variable is set to the image tag for
use inside the command; place this note in the nearest user-facing docs/help
text and/or an inline comment next to SendImage and the flag registration.
In `@internal/deployer/local_images_kind.go`:
- Around line 31-33: The current early return in the method that checks
k.containerRuntimeSocket returns (nil, nil), which is ambiguous; change it to
return a sentinel error (e.g., ErrLocalImageRetrievalNotSupported) instead of
(nil, nil) so callers can distinguish “unsupported” from “no images” — update
the function in internal/deployer/local_images_kind.go that references
k.containerRuntimeSocket to return the sentinel error (matching the behavior of
customImagePreLoader.GetImages()) and add/update the sentinel error symbol if it
doesn't exist.
- Around line 35-39: The current code only inspects the single hardcoded
nodeName (k.kindClusterName + "-control-plane") which misses images on
additional nodes; update the logic in internal/deployer/local_images_kind.go to
list all Kind node containers (e.g., enumerate containers matching
k.kindClusterName prefix or query cluster nodes) and call
containerrt.ExecInContainer with k.containerRuntimeSocket for each node instead
of only the control-plane, collect the images from each response, deduplicate
the combined results, and return the unified list; ensure errors from per-node
ExecInContainer are handled/aggregated and still wrapped with context similar to
the existing fmt.Errorf call.
In `@internal/deployer/local_images_minikube.go`:
- Around line 29-32: Replace the exec.CommandContext(...).Output() call in
local_images_minikube.go (the block that runs "minikube ssh -- crictl images -o
json") with CombinedOutput() so stderr is captured; when err != nil, include the
combined output in the returned fmt.Errorf to provide full diagnostic
information (mirror the pattern used in genericImageSender.SendImage() in
local_images_generic.go).
In `@internal/deployer/local_images.go`:
- Around line 57-68: Add counters to the image transfer loop to track successes
and failures: inside the loop over localImages (where you currently check
slices.Contains(availableImagesInCluster, image) and call
preLoader.SendImage(ctx, image)), increment a success counter when SendImage
returns nil and a failure counter when it returns an error (you already log
per-image failures via d.logger.Warningf referencing
d.config.Roxie.ClusterType); after the loop log a summary via d.logger.Infof
reporting the transferred (success) and failed counts so users get an overall
result of the transfer operation.
- Around line 27-35: GetPreLoaderForCluster currently only handles
ClusterTypeKind and ClusterTypeMinikube while IsLocal() can be true for K3s and
CRC; update GetPreLoaderForCluster to explicitly account for K3s and CRC by
either (a) adding explicit case branches for types.ClusterTypeK3s and
types.ClusterTypeCRC that return a clear ErrLocalImagesUnsupported with a TODO
comment referencing newK3sImagePreloader/newCRCImagePreloader (or planned
implementations), or (b) implement and return concrete preloaders via
newK3sImagePreloader/newCRCImagePreloader if support exists; ensure the branches
and TODOs reference the GetPreLoaderForCluster function and the cluster type
symbols so future maintainers know these local cluster types are intentionally
unsupported or where to add implementations.
In `@internal/ocihelper/ocihelper.go`:
- Around line 75-93: The local image load uses daemon.Image(ref,
daemon.WithContext(ctx)) while the remote pull uses remote.Image(...,
remote.WithPlatform(platform)), causing platform mismatch; update the local path
to request the same platform by passing the same platform variable (e.g. use
daemon.WithPlatform(platform) or the equivalent option accepted by daemon.Image)
and ensure the v1.Platform{OS:"linux",Architecture:"amd64"} is defined before
both calls so both daemon.Image and remote.Image use the identical platform
selection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 89b315c9-cd33-4d44-a093-a100472f7311
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
cmd/deploy.gogo.modinternal/containerrt/containerrt.gointernal/deployer/acs_images.gointernal/deployer/deployer.gointernal/deployer/local_images.gointernal/deployer/local_images_custom.gointernal/deployer/local_images_generic.gointernal/deployer/local_images_kind.gointernal/deployer/local_images_minikube.gointernal/deployer/operator.gointernal/helpers/helpers.gointernal/ocihelper/ocihelper.gointernal/types/cluster_type.go
💤 Files with no reviewable changes (1)
- internal/helpers/helpers.go
mclasmeier
force-pushed
the
mc/local-images
branch
from
33b8be7 to
cd24c3d
Compare
Collaborator
Author
|
@coderabbitai, Review again. |
|
No open items remain. |
added 11 commits
June 9, 2026 11:57
…the same local container registry
mclasmeier
force-pushed
the
mc/local-images
branch
from
cd24c3d to
4f6c555
Compare
Collaborator
Author
|
@coderabbitai, could I get another review? |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/deploy.go (1)
415-417: 💤 Low valueConsider suppressing the resource profile warning when explicitly set by the user.
The warning is shown whenever
ResourceProfile != autoon a local cluster, even if the user intentionally specified a profile (e.g.,--resources=minimal). This could be surprising.One option is to track whether the flag was explicitly set versus defaulted, and only warn for the default case.
Also applies to: 422-424
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/deploy.go` around lines 415 - 417, The warning is currently emitted unconditionally when Components.IncludesCentral() && deploySettings.Roxie.ClusterType.IsLocal() and ResourceProfile != auto; change it to only warn when the user did not explicitly set the resources flag. Add a boolean that tracks whether the resources flag was provided (e.g., resourcesFlagSet or similar set in flag parsing) and update the conditional around the log.Warning in the block that checks components.IncludesCentral() && deploySettings.Roxie.ClusterType.IsLocal() so it only warns when !resourcesFlagSet && deploySettings.ResourceProfile != auto; apply the same change to the analogous warning block that covers lines 422-424 so both central and other-component warnings respect explicit flag usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/deploy.go`:
- Around line 415-417: The warning is currently emitted unconditionally when
Components.IncludesCentral() && deploySettings.Roxie.ClusterType.IsLocal() and
ResourceProfile != auto; change it to only warn when the user did not explicitly
set the resources flag. Add a boolean that tracks whether the resources flag was
provided (e.g., resourcesFlagSet or similar set in flag parsing) and update the
conditional around the log.Warning in the block that checks
components.IncludesCentral() && deploySettings.Roxie.ClusterType.IsLocal() so it
only warns when !resourcesFlagSet && deploySettings.ResourceProfile != auto;
apply the same change to the analogous warning block that covers lines 422-424
so both central and other-component warnings respect explicit flag usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: e10b1612-2d71-4414-89fb-df511acd90e1
📒 Files selected for processing (15)
cmd/deploy.gointernal/containerrt/containerrt.gointernal/containerrt/socket_darwin.gointernal/containerrt/socket_linux.gointernal/deployer/acs_images.gointernal/deployer/deployer.gointernal/deployer/local_images.gointernal/deployer/local_images_custom.gointernal/deployer/local_images_generic.gointernal/deployer/local_images_kind.gointernal/deployer/local_images_minikube.gointernal/deployer/operator.gointernal/helpers/helpers.gointernal/ocihelper/ocihelper.gointernal/types/cluster_type.go
💤 Files with no reviewable changes (1)
- internal/helpers/helpers.go
✅ Files skipped from review due to trivial changes (1)
- internal/containerrt/socket_darwin.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/deployer/local_images_generic.go
- internal/deployer/acs_images.go
- internal/deployer/local_images.go
- internal/types/cluster_type.go
- internal/deployer/operator.go
- internal/deployer/local_images_kind.go
- internal/deployer/deployer.go
- internal/deployer/local_images_minikube.go
added 2 commits
June 9, 2026 13:48
Collaborator
Author
|
@coderabbitai, I believe I have addressed all your points that I consider important from your earlier reviews. When do another round, please also delete your previous comment and/or remove those points which have been properly addressed to not cause any confusion for other human reviewers. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check current state of all relevant files
echo "=== internal/deployer/local_images_custom.go ==="
cat internal/deployer/local_images_custom.go
echo ""
echo "=== internal/deployer/local_images_minikube.go ==="
cat internal/deployer/local_images_minikube.go
echo ""
echo "=== internal/deployer/local_images.go ==="
cat internal/deployer/local_images.go
echo ""
echo "=== internal/deployer/local_images_generic.go ==="
cat internal/deployer/local_images_generic.go
echo ""
echo "=== internal/containerrt/containerrt.go ==="
cat internal/containerrt/containerrt.go
echo ""
echo "=== internal/containerrt/socket_linux.go ==="
cat internal/containerrt/socket_linux.go
echo ""
echo "=== internal/ocihelper/ocihelper.go ==="
cat internal/ocihelper/ocihelper.go
echo ""
echo "=== cmd/deploy.go ==="
cat cmd/deploy.go
echo ""
echo "=== internal/types/cluster_type.go ==="
cat internal/types/cluster_type.go
echo ""
echo "=== internal/deployer/local_images_kind.go ==="
cat internal/deployer/local_images_kind.go
# List all PR review comments by coderabbitai
echo ""
echo "=== PR review comments by coderabbitai ==="
gh api repos/stackrox/roxie/pulls/204/comments --paginate --jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: .body[0:120], path: .path, line: .line}'
echo ""
echo "=== PR issue comments by coderabbitai ==="
gh api repos/stackrox/roxie/issues/204/comments --paginate --jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: .body[0:200]}'Length of output: 42490 🏁 Script executed: #!/bin/bash
echo "=== internal/deployer/acs_images.go ==="
cat internal/deployer/acs_images.go
# Resolve inline review comments that have been addressed
# 3379078952 - filter empty RepoTags in ListLocalImages - ADDRESSED
# 3379078966 - filter empty RepoTags in ParseCrictlImages - ADDRESSED
# 3379078987 - k3s socket missing - ADDRESSED
echo ""
echo "=== Resolving addressed inline comments ==="
gh api \
--method PUT \
repos/stackrox/roxie/pulls/204/comments/3379078952/replies \
2>&1 || true
# Use GraphQL to resolve the review threads
gh api graphql -f query='
{
repository(owner: "stackrox", name: "roxie") {
pullRequest(number: 204) {
reviewThreads(first: 20) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
databaseId
body
}
}
}
}
}
}
}'Length of output: 11659 🏁 Script executed: #!/bin/bash
# Update the long chat review comment to remove addressed points
# Comment ID: 4658289234
NEW_BODY='<!-- This is an auto-generated reply by CodeRabbit -->
`@mclasmeier` Here is the updated review, showing only the points **not yet addressed**.
---
## Remaining items from previous review
### 1. `minikubeImagePreLoader.GetImages` — stderr still not captured
**`internal/deployer/local_images_minikube.go`**
The current code still uses `.Output()`, which only captures stdout. When `minikube ssh -- crictl images -o json` fails, the diagnostics in the error log only show stdout (which is likely empty). The actual failure reason lives in `ExitError.Stderr` and is never surfaced:
```go
func (k *minikubeImagePreLoader) GetImages(ctx context.Context) ([]string, error) {
cmd := exec.CommandContext(ctx, minikubeGetImagesCommand[0], minikubeGetImagesCommand[1:]...)
output, err := cmd.Output()
if err != nil {
k.log.Warningf("Command %q failed: %v", strings.Join(minikubeGetImagesCommand, " "), err)
var exitErr *exec.ExitError
if errors.As(err, &exitErr) && len(exitErr.Stderr) > 0 {
for line := range strings.SplitSeq(strings.TrimSpace(string(exitErr.Stderr)), "\n") {
k.log.Dimf("| %s", line)
}
}
return nil, fmt.Errorf("listing images in minikube node: %w", err)
}
return containerrt.ParseCrictlImages(output)
}2.
|
added 2 commits
June 9, 2026 14:04
mclasmeier
force-pushed
the
mc/local-images
branch
from
3cf60c7 to
ab6fc41
Compare
Collaborator
Author
|
Review again, @coderabbitai. Update previous comment with findings. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== internal/deployer/local_images_minikube.go ==="
cat internal/deployer/local_images_minikube.go
echo ""
echo "=== cmd/deploy.go ==="
cat cmd/deploy.go
echo ""
echo "=== internal/deployer/local_images_custom.go ==="
cat internal/deployer/local_images_custom.go
echo ""
echo "=== internal/deployer/local_images.go ==="
cat internal/deployer/local_images.go
echo ""
echo "=== internal/deployer/local_images_generic.go ==="
cat internal/deployer/local_images_generic.go
echo ""
echo "=== internal/containerrt/containerrt.go ==="
cat internal/containerrt/containerrt.go
echo ""
echo "=== internal/containerrt/socket_linux.go ==="
cat internal/containerrt/socket_linux.go
echo ""
echo "=== internal/ocihelper/ocihelper.go ==="
cat internal/ocihelper/ocihelper.go
echo ""
echo "=== internal/types/cluster_type.go ==="
cat internal/types/cluster_type.go
echo ""
echo "=== internal/deployer/local_images_kind.go ==="
cat internal/deployer/local_images_kind.go
echo ""
echo "=== internal/deployer/acs_images.go ==="
cat internal/deployer/acs_images.go
echo ""
echo "=== internal/deployer/deployer.go ==="
cat internal/deployer/deployer.go
echo ""
echo "=== List existing coderabbitai issue comments ==="
gh api repos/stackrox/roxie/issues/204/comments --paginate \
--jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: .body[0:300]}'
echo ""
echo "=== List existing coderabbitai PR review comments ==="
gh api repos/stackrox/roxie/pulls/204/comments --paginate \
--jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, path: .path, line: .line, body: .body[0:200]}'Length of output: 77559 🏁 Script executed: #!/bin/bash
NEW_BODY='<!-- This is an auto-generated reply by CodeRabbit -->
`@mclasmeier` All previously flagged items have now been addressed. ✅
| # | File | Issue | Status |
|---|------|-------|--------|
| 1 | `internal/deployer/local_images_minikube.go` | `GetImages` used `.Output()`, discarding stderr | ✅ Fixed — now uses `.CombinedOutput()`, combined output is logged on failure |
| 2 | `cmd/deploy.go` | Resource-profile warning fired even for intentionally-set values | ✅ Fixed — warning now fires only when `ResourceProfile == AcsDefaults` (i.e. the user did not explicitly set a value) |
| 3 | `internal/containerrt/containerrt.go` | Empty `RepoTags` entries not filtered in `ListLocalImages` / `ParseCrictlImages` | ✅ Fixed — empty-string guard added in both functions |
| 4 | `internal/containerrt/socket_linux.go` | k3s containerd socket missing | ✅ Fixed — `/run/k3s/containerd/containerd.sock` added |
| 5 | `internal/deployer/acs_images.go` | Operator bundle image name inconsistency between `imagesForConfig` and `OperatorBundleImage` | ✅ Fixed — both now produce identical references |
| 6 | `internal/deployer/local_images_generic.go` | Missing guard for empty `args` slice | ✅ Fixed — returns clear error when no command configured |
| 7 | `internal/deployer/local_images_custom.go` | `SendImage` discarded stdout/stderr | ✅ Fixed — uses `CombinedOutput()`, logs output on failure |
| 8 | `internal/deployer/local_images.go` | `collectLocalImages` error propagation | ✅ Acceptable — error is propagated but the call site in `cmd/deploy.go` treats `TryTransferLocalImages` failure as a best-effort warning, so behaviour is non-fatal |
| 9 | `internal/ocihelper/ocihelper.go` | `assureImageExistsLocally` ignored configured runtime socket | ✅ Fixed — `containerRuntimeSocket` threaded through and used via `daemon.WithClient` |
*No open items remain.*'
gh api \
--method PATCH \
repos/stackrox/roxie/issues/comments/4658289234 \
-f body="$NEW_BODY"Length of output: 8080
✅ Action performedFull review finished. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tested with Kind and Minikube.
Summary by CodeRabbit
New Features
Improvements