Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions pkg/container/container_podman.go
Original file line number Diff line number Diff line change
@@ -1,15 +1,40 @@
package container

import (
"bytes"
"encoding/base64"
"fmt"
"os"
"path/filepath"
"runtime"
"strings"

logger "github.com/sirupsen/logrus"
)

// checkRosettaEnabled verifies if Rosetta is enabled in Podman on macOS
// This is a non-blocking check that provides a hint to the user if Rosetta is not configured
func checkRosettaEnabled() {
// Rosetta is only relevant on Apple Silicon (arm64) macOS; skip on other platforms
if runtime.GOOS != "darwin" || runtime.GOARCH != "arm64" {
return
}

checkCmd := createCommand(PODMAN, "machine", "ssh", "ls /proc/sys/fs/binfmt_misc/")
var out bytes.Buffer
checkCmd.Stdout = &out
checkCmd.Stderr = nil

if err := checkCmd.Run(); err != nil {
// Silently skip if we can't check
return
Comment on lines +23 to +30
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Verify the current argv shape in the code.
rg -n -C2 'createCommand\(PODMAN,\s*"machine",\s*"ssh"' pkg/container/container_podman.go

# Verify Podman docs for podman machine ssh syntax and examples.
curl -fsSL https://docs.podman.io/en/latest/markdown/podman-machine-ssh.1.html | sed -n '/SYNOPSIS/,/DESCRIPTION/p'
curl -fsSL https://docs.podman.io/en/latest/markdown/podman-machine-ssh.1.html | sed -n '/EXAMPLES/,/SEE ALSO/p'

Repository: openshift/backplane-cli

Length of output: 1413


Split the remote ssh command into separate argv tokens.

podman machine ssh is documented as [name] [command [arg …]], and the official examples pass the remote command and its arguments as separate tokens. Passing "ls /proc/sys/fs/binfmt_misc/" as one token will cause Podman to treat the entire string as the executable name, making this probe fail and silently skip the warning.

Suggested fix
-	checkCmd := createCommand(PODMAN, "machine", "ssh", "ls /proc/sys/fs/binfmt_misc/")
+	checkCmd := createCommand(PODMAN, "machine", "ssh", "ls", "/proc/sys/fs/binfmt_misc/")
🤖 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 `@pkg/container/container_podman.go` around lines 23 - 30, The remote ssh probe
in container_podman.go passes the whole command as a single argv token, causing
Podman to treat it as the executable; update the checkCmd invocation (where
checkCmd := createCommand(PODMAN, "machine", "ssh", "ls
/proc/sys/fs/binfmt_misc/") is created) to pass the remote command and its
arguments as separate tokens (e.g., "ls", "/proc/sys/fs/binfmt_misc/") so Podman
runs the intended command on the remote machine; leave the rest of the check
(Stdout buffer, Stderr handling, Run error handling) unchanged.

}
Comment thread
feichashao marked this conversation as resolved.

if !strings.Contains(out.String(), "rosetta") {
logger.Warnf("Rosetta does not appear to be enabled in Podman. For better compatibility with x86_64 images on Apple Silicon, please configure Rosetta. See https://github.com/openshift/backplane-cli/blob/main/docs/macOS.md for setup instructions.")
}
}

type podmanLinux struct {
fileMountDir string
}
Expand Down Expand Up @@ -82,6 +107,8 @@ func podmanRunConsoleContainer(containerName string, port string, consoleArgs []
}

func (ce *podmanMac) RunConsoleContainer(containerName string, port string, consoleArgs []string, envVars []EnvVar) error {
// Check if Rosetta is enabled for better compatibility
checkRosettaEnabled()
return podmanRunConsoleContainer(containerName, port, consoleArgs, envVars)
}

Expand Down
113 changes: 111 additions & 2 deletions pkg/container/container_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package container
import (
"os"
"os/exec"
"runtime"
"strings"
"testing"

Expand Down Expand Up @@ -100,6 +101,113 @@ var _ = Describe("console container implementation", func() {
})
})

Context("when checking Rosetta on macOS Podman", func() {
It("should execute podman machine ssh command on darwin/arm64", func() {
if runtime.GOOS == "darwin" && runtime.GOARCH == "arm64" {
capturedCommands = nil
checkRosettaEnabled()
Expect(len(capturedCommands)).To(Equal(1))
command := capturedCommands[0]
Expect(command[0]).To(Equal(PODMAN))
Expect(command[1]).To(Equal("machine"))
Expect(command[2]).To(Equal("ssh"))
Expect(strings.Join(command[3:], " ")).To(Equal("ls /proc/sys/fs/binfmt_misc/"))
} else {
Skip("Rosetta check only runs on darwin/arm64")
}
})
Comment on lines +104 to +118
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Exercise the warning path, not just the podman machine ssh argv.

These specs prove that checkRosettaEnabled() shells out on the current host, but they still do not cover the user-visible behavior added in pkg/container/container_podman.go:17-36: warning when stdout lacks rosetta, and no warning when it is present. With createCommand hardwired to exec.Command("true"), that branch can regress without any test here failing. Consider making the platform/output decision injectable so both warning and no-warning cases run deterministically on any runner.

As per coding guidelines, **/*_test.go: "Review test coverage and edge cases" and "Ensure tests are deterministic and not flaky".

🤖 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 `@pkg/container/container_test.go` around lines 104 - 118, The tests only
assert that checkRosettaEnabled invoked podman but don't exercise the
warning/no-warning branches; make the command creation/output behavior
injectable so tests can deterministically simulate stdout containing "rosetta"
and not containing it. Refactor pkg/container/container_podman.go to expose or
accept a createCommand factory (or a package-level variable used by
checkRosettaEnabled) that tests can replace; in container_test.go set that
factory to stubs that return controlled stdout (one with "rosetta" and one
without) and assert the expected warning emission (and absence) from
checkRosettaEnabled, covering both paths regardless of runtime.GOOS/GOARCH.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too heavy, skip this.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@feichashao, understood — acknowledged and moving on. The current tests still validate that the correct podman machine ssh command is invoked, which is a reasonable coverage level for this change.

It("should skip the check on non-darwin platforms", func() {
if runtime.GOOS != "darwin" {
capturedCommands = nil
checkRosettaEnabled()
Expect(len(capturedCommands)).To(Equal(0))
} else {
Skip("This test only runs on non-darwin platforms")
}
})
It("should skip the check on non-arm64 architectures", func() {
if runtime.GOOS == "darwin" && runtime.GOARCH != "arm64" {
capturedCommands = nil
checkRosettaEnabled()
Expect(len(capturedCommands)).To(Equal(0))
} else {
Skip("This test only runs on darwin with non-arm64 architectures")
}
})
})

Context("when running console container on macOS", func() {
ce := podmanMac{}
It("should check Rosetta before running the container on darwin/arm64", func() {
if runtime.GOOS == "darwin" && runtime.GOARCH == "arm64" {
mockOcmInterface.EXPECT().GetPullSecret().Return(pullSecret, nil).AnyTimes()
capturedCommands = nil
args := []string{"arg1"}
envvars := []EnvVar{{Key: "testkey", Value: "testval"}}
err := ce.RunConsoleContainer("console", "8888", args, envvars)
Expect(err).To(BeNil())
// Count Rosetta check commands
rosettaCheckCount := 0
for _, cmd := range capturedCommands {
if len(cmd) >= 3 && cmd[0] == PODMAN && cmd[1] == "machine" && cmd[2] == "ssh" {
rosettaCheckCount++
}
}
// Should have exactly 1 Rosetta check
Expect(rosettaCheckCount).To(Equal(1))
// First command should be Rosetta check
rosettaCheckCmd := capturedCommands[0]
Expect(rosettaCheckCmd[0]).To(Equal(PODMAN))
Expect(rosettaCheckCmd[1]).To(Equal("machine"))
Expect(rosettaCheckCmd[2]).To(Equal("ssh"))
// Last command should be the actual container run
runCmd := capturedCommands[len(capturedCommands)-1]
fullCommand := strings.Join(runCmd, " ")
Expect(fullCommand).To(ContainSubstring("arg1"))
Expect(fullCommand).To(ContainSubstring("--env"))
Expect(fullCommand).To(ContainSubstring("testkey=testval"))
} else {
Skip("Rosetta check only runs on darwin/arm64")
}
})
It("should skip Rosetta check on non-darwin platforms", func() {
if runtime.GOOS != "darwin" {
mockOcmInterface.EXPECT().GetPullSecret().Return(pullSecret, nil).AnyTimes()
capturedCommands = nil
args := []string{"arg1"}
envvars := []EnvVar{{Key: "testkey", Value: "testval"}}
err := ce.RunConsoleContainer("console", "8888", args, envvars)
Expect(err).To(BeNil())
// Should only have 1 command for running container (no Rosetta check)
Expect(len(capturedCommands)).To(Equal(1))
fullCommand := strings.Join(capturedCommands[0], " ")
Expect(fullCommand).To(ContainSubstring("arg1"))
Expect(fullCommand).To(ContainSubstring("--env"))
Expect(fullCommand).To(ContainSubstring("testkey=testval"))
} else {
Skip("This test only runs on non-darwin platforms")
}
})
It("should skip Rosetta check on darwin with non-arm64 architectures", func() {
if runtime.GOOS == "darwin" && runtime.GOARCH != "arm64" {
mockOcmInterface.EXPECT().GetPullSecret().Return(pullSecret, nil).AnyTimes()
capturedCommands = nil
args := []string{"arg1"}
envvars := []EnvVar{{Key: "testkey", Value: "testval"}}
err := ce.RunConsoleContainer("console", "8888", args, envvars)
Expect(err).To(BeNil())
// Should only have 1 command for running container (no Rosetta check)
Expect(len(capturedCommands)).To(Equal(1))
fullCommand := strings.Join(capturedCommands[0], " ")
Expect(fullCommand).To(ContainSubstring("arg1"))
Expect(fullCommand).To(ContainSubstring("--env"))
Expect(fullCommand).To(ContainSubstring("testkey=testval"))
} else {
Skip("This test only runs on darwin with non-arm64 architectures")
}
})
})

Context("when running console container", func() {
ce := podmanMac{}
It("should pass argments and environment variable if specified", func() {
Expand All @@ -109,8 +217,9 @@ var _ = Describe("console container implementation", func() {
envvars := []EnvVar{{Key: "testkey", Value: "testval"}}
err := ce.RunConsoleContainer("console", "8888", args, envvars)
Expect(err).To(BeNil())
Expect(len(capturedCommands)).To(Equal(1))
fullCommand := strings.Join(capturedCommands[0], " ")
Expect(len(capturedCommands)).To(BeNumerically(">=", 1))
// Find the run command (should be the last one)
fullCommand := strings.Join(capturedCommands[len(capturedCommands)-1], " ")
// arg
Expect(fullCommand).To(ContainSubstring("arg1"))
// env var
Expand Down