feat(runtime/podman): implement podman runtime with hardcoded image#83
Open
feloy wants to merge 23 commits intokortex-hub:mainfrom
Open
feat(runtime/podman): implement podman runtime with hardcoded image#83feloy wants to merge 23 commits intokortex-hub:mainfrom
feloy wants to merge 23 commits intokortex-hub:mainfrom
Conversation
Created a new Podman runtime implementation that integrates with the existing runtime registration system. The runtime checks for podman CLI availability and is only registered when podman is installed. Also introduced a new system package following the module design pattern to provide testable system-level operations like command existence checking. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Fork PRs don't have access to repository secrets, so Codecov uploads will fail. Setting fail_ci_if_error to false allows the workflow to continue successfully even when the upload fails. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
feat(runtime): add podman runtime with system utilities module
…ning Adds full container creation support to the Podman runtime including: - Containerfile generation with user-specific UID/GID - Image building using Fedora base with development tools - Container creation with workspace mounts and configuration - Environment variable and secret handling - Dependency path validation to prevent directory escapes Extends system module with Getuid/Getgid for user ID management. Implements StorageAware interface for runtime data persistence. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Removes stdout/stderr redirection from podman build and container create operations to prevent verbose output from being displayed to the user during container provisioning. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Configuration paths use forward slashes for cross-platform compatibility, but Windows uses backslashes. This fix properly converts config paths to OS-specific separators for host mounts while keeping container paths as forward slashes (Linux). Tests now use t.TempDir() for cross-platform compatibility. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
feat(runtime/podman): implement Create method with container provisioning
Adds the Start method to start podman containers and retrieve their state. Introduces an exec package to abstract podman command execution, improving testability by allowing fake implementations in tests. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
feat(runtime/podman): implement Start method with exec abstraction
Implements the Stop method for the podman runtime, allowing containers to be stopped gracefully. Follows the same pattern as the Start method with proper validation and error handling using the executor abstraction. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Implements the Info method for the podman runtime, allowing retrieval of container state and metadata. Moves getContainerInfo helper and its tests to info.go/info_test.go for better organization since this helper is primarily used by the Info method. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
feat(runtime/podman): implement Stop and Info methods
Implements the Remove method to delete Podman containers and clean up resources. The method validates container state, prevents removal of running containers, and provides idempotent behavior when containers don't exist. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
feat(runtime/podman): implement Remove method
The Delete method was ignoring errors from runtime.Remove(), which could leave the runtime instance intact while removing the manager's record, causing inconsistent state. Now Delete properly returns errors if runtime removal fails, preventing partial deletions. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
fix(instances): handle runtime Remove errors in Delete method
…ions Documents the Podman runtime configuration including the Fedora base image, installed development tools, claude user setup with limited sudo permissions, and Claude Code as the default AI agent. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
docs(runtime/podman): document container image, packages, and permissions
This reverts commit cc96f30. Signed-off-by: Philippe Martin <phmartin@redhat.com>
feloy
force-pushed
the
podman-runtime-branch
branch
from
0410729 to
1bc3199
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Podman runtime (create/start/stop/remove/info) with a Podman CLI executor + fake, a system abstraction for UID/GID, Podman runtime registration, stricter manager.Delete error handling and tests, and README documentation for the Podman runtime. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as Client/CLI
participant Manager as Instance Manager
participant PodmanRT as Podman Runtime
participant FS as Host Filesystem
participant PodmanCLI as podman CLI
User->>CLI: init --runtime podman
CLI->>Manager: Create(name, source_path, params)
Manager->>PodmanRT: Create(params)
PodmanRT->>PodmanRT: validate params & deps
PodmanRT->>FS: create instance dir & write Containerfile/sudoers
PodmanRT->>PodmanCLI: podman build --build-arg UID/GID -t image
PodmanCLI-->>PodmanRT: image created
PodmanRT->>PodmanCLI: podman create (envs, secrets, mounts, image, sleep)
PodmanCLI-->>PodmanRT: container created (id)
PodmanRT-->>Manager: RuntimeInfo{State: "created", container_id: id}
Manager-->>CLI: success
sequenceDiagram
actor User
participant CLI as Client/CLI
participant Manager as Instance Manager
participant PodmanRT as Podman Runtime
participant PodmanCLI as podman CLI
User->>CLI: start <id>
CLI->>Manager: Start(id)
Manager->>PodmanRT: Start(id)
PodmanRT->>PodmanCLI: podman start <id>
PodmanRT->>PodmanCLI: podman inspect --format ...
PodmanCLI-->>PodmanRT: state|image
PodmanRT-->>Manager: RuntimeInfo{State: "running"}
Manager-->>CLI: success
User->>CLI: delete instance
CLI->>Manager: Delete(name)
Manager->>PodmanRT: Remove(container_id)
PodmanRT->>PodmanCLI: podman inspect ...
alt container running
PodmanRT-->>Manager: error (still running)
Manager-->>CLI: error
else container stopped or not found
PodmanRT->>PodmanCLI: podman rm (if exists)
PodmanRT-->>Manager: nil
Manager->>Manager: remove instance from storage
Manager-->>CLI: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
pkg/runtime/podman/exec/exec.go (1)
46-55: Consider capturing stderr for improved error diagnostics.When podman commands fail, the error message from podman (written to stderr) is lost. This can make debugging difficult. Consider using
CombinedOutput()or capturing stderr separately to include in error messages.♻️ Example approach using CombinedOutput for Output method
// Output executes a podman command and returns its standard output. func (e *executor) Output(ctx context.Context, args ...string) ([]byte, error) { cmd := exec.CommandContext(ctx, "podman", args...) - return cmd.Output() + output, err := cmd.CombinedOutput() + if err != nil { + return nil, fmt.Errorf("%w: %s", err, string(output)) + } + return output, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/podman/exec/exec.go` around lines 46 - 55, The Run and Output methods on executor currently drop podman's stderr, losing useful error details; change Output to use cmd.CombinedOutput() and return that output (so stderr is included) and change Run to capture combined output (e.g., via cmd.CombinedOutput() or by setting cmd.Stderr to a buffer) and, on error, wrap/return an error that includes the combined stdout+stderr text; update functions executor.Run and executor.Output to surface podman stderr in returned errors or outputs for better diagnostics.pkg/runtime/podman/podman_test.go (1)
97-109: Minor: Zero value handling inGetuid/Getgidcould mask root user scenarios.The current implementation treats
uid == 0as "not configured" and returns 1000, but 0 is also the actual UID for root. If future tests need to simulate root user behavior, this could be problematic. Consider using a pointer or explicit flag to distinguish "not set" from "set to zero".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/podman/podman_test.go` around lines 97 - 109, The Getuid/Getgid helpers treat a zero value as "unset" and return 1000, which prevents simulating UID/GID 0 (root); update the fakeSystem struct to distinguish "not set" from zero (e.g., change uid and gid to *int or add explicit bool flags like uidSet/gidSet), then modify Getuid and Getgid to check for nil or the flags instead of value==0 and return the stored 0 when explicitly set; also update any test constructors/initializers for fakeSystem to set the pointer or flags when intending to simulate specific UIDs/GIDs.pkg/system/system_test.go (1)
35-47: Addt.Parallel()to subtests.Per coding guidelines, all test functions should call
t.Parallel()as the first line. The subtests are missing this call.♻️ Proposed fix
t.Run("existing command", func(t *testing.T) { + t.Parallel() // 'go' should exist since we're running go test if !sys.CommandExists("go") { t.Error("Expected 'go' command to exist") } }) t.Run("non-existing command", func(t *testing.T) { + t.Parallel() // This command should not exist if sys.CommandExists("this-command-definitely-does-not-exist-xyz123") { t.Error("Expected non-existing command to return false") } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/system/system_test.go` around lines 35 - 47, The two subtests created with t.Run in system_test.go ("existing command" and "non-existing command") are missing t.Parallel(); add t.Parallel() as the first statement inside each subtest's func(t *testing.T) body so both subtests run in parallel (i.e., inside the funcs that call sys.CommandExists, call t.Parallel() immediately before any assertions).pkg/runtime/podman/info.go (1)
44-54: Consider handling container-not-found errors specifically.When
podman inspectfails because the container doesn't exist, the error should ideally be wrapped withruntime.ErrInstanceNotFoundto allow callers to distinguish this case from other failures (e.g., permission issues, podman crashes).Additionally, the pipe
|delimiter could theoretically appear in image names, though this is rare. A more robust approach would use JSON output or a delimiter less likely to appear in values.♻️ Example for container-not-found handling
output, err := p.executor.Output(ctx, "inspect", "--format", "{{.Id}}|{{.State.Status}}|{{.ImageName}}", id) if err != nil { + // Check if the error indicates container not found + if strings.Contains(err.Error(), "no such container") || + strings.Contains(err.Error(), "no such object") { + return runtime.RuntimeInfo{}, fmt.Errorf("%w: %s", runtime.ErrInstanceNotFound, id) + } return runtime.RuntimeInfo{}, fmt.Errorf("failed to inspect container: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/podman/info.go` around lines 44 - 54, The inspect call currently uses p.executor.Output and splits on "|" (see use of p.executor.Output(... "inspect" ... id) and the fields variable), which fails to distinguish a missing container and is brittle for delimiters; change to request JSON from podman (use --format '{{json .}}'), unmarshal into a small struct containing Id, State.Status, and ImageName, and build the runtime.RuntimeInfo from those fields instead of splitting; detect podman’s "no such container" error text from the command error and wrap/return runtime.ErrInstanceNotFound for that case (where the existing code returns fmt.Errorf("failed to inspect container: %w", err)), and remove the brittle string split and len(fields) check.pkg/runtime/podman/remove_test.go (1)
235-247: Usestrings.Containsfrom stdlib instead of custom helpers.The custom
containsandstringContainsfunctions duplicatestrings.Contains. The standard library version is more readable, tested, and optimized.♻️ Proposed fix
-// contains checks if a string contains a substring -func contains(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || len(s) > len(substr) && stringContains(s, substr)) -} - -func stringContains(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false -}Then update line 133 to use the standard library:
- if !contains(err.Error(), expectedMsg) { + if !strings.Contains(err.Error(), expectedMsg) {Don't forget to add
"strings"to the imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/podman/remove_test.go` around lines 235 - 247, Replace the custom contains and stringContains helpers with the standard library strings.Contains: remove the functions contains and stringContains, add "strings" to the imports, and update any call sites that use contains (e.g., the test that currently calls contains) to call strings.Contains(s, substr) instead.pkg/runtime/podman/create_test.go (2)
481-484: Check errors fromos.MkdirAll.Same issue as noted earlier — errors from
os.MkdirAllshould be checked.♻️ Proposed fix
- os.MkdirAll(currentDir, 0755) - os.MkdirAll(mainDir, 0755) + if err := os.MkdirAll(currentDir, 0755); err != nil { + t.Fatalf("Failed to create currentDir: %v", err) + } + if err := os.MkdirAll(mainDir, 0755); err != nil { + t.Fatalf("Failed to create mainDir: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/podman/create_test.go` around lines 481 - 484, The two os.MkdirAll calls that create currentDir and mainDir do not check for errors; change each to capture the returned error (err := os.MkdirAll(...)) and fail the test on error (e.g., if err != nil { t.Fatalf("failed to create %s: %v", currentDir, err) } and similarly for mainDir) so failures surface in the test; reference the existing os.MkdirAll calls and the currentDir/mainDir variables in your change.
374-376: Check errors fromos.MkdirAll.
os.MkdirAllcan fail (e.g., permission issues), and ignoring the error could lead to confusing test failures later when the directories don't exist.♻️ Proposed fix
- os.MkdirAll(currentDir, 0755) - os.MkdirAll(mainDir, 0755) - os.MkdirAll(sharedDir, 0755) + if err := os.MkdirAll(currentDir, 0755); err != nil { + t.Fatalf("Failed to create currentDir: %v", err) + } + if err := os.MkdirAll(mainDir, 0755); err != nil { + t.Fatalf("Failed to create mainDir: %v", err) + } + if err := os.MkdirAll(sharedDir, 0755); err != nil { + t.Fatalf("Failed to create sharedDir: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/podman/create_test.go` around lines 374 - 376, The three os.MkdirAll calls for currentDir, mainDir, and sharedDir are not checking errors; update the test to capture and handle each error (e.g., if err := os.MkdirAll(currentDir, 0755); err != nil { t.Fatalf("mkdir %s: %v", currentDir, err) } or use require.NoError(t, err) if testify is available) so any failure to create those directories fails the test immediately with a helpful message; apply the same pattern to mainDir and sharedDir.pkg/runtime/podman/create.go (1)
97-97: Sudoers file should have more restrictive permissions.The sudoers file is created with
0644(world-readable). Standard practice for sudoers files is0440or0400to prevent non-root users from reading the security configuration. While this file is copied into the container and the risk is limited, using correct permissions is a good security habit.🛡️ Proposed fix
- if err := os.WriteFile(sudoersPath, []byte(sudoersContent), 0644); err != nil { + if err := os.WriteFile(sudoersPath, []byte(sudoersContent), 0440); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/podman/create.go` at line 97, The sudoers file is being written with world-readable permissions via os.WriteFile(sudoersPath, []byte(sudoersContent), 0644); update the file mode to a restrictive one (e.g., 0440 or 0400) when writing to sudoersPath to prevent non-root users from reading it; locate the os.WriteFile call that uses sudoersPath/sudoersContent in create.go and replace the mode value accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/runtime/podman/create_test.go`:
- Around line 425-427: The test uses a hardcoded SourcePath (params :=
runtime.CreateParams{Name: "test-workspace", SourcePath: "/path/to/source"})
which is not cross-platform; change it to use a temporary directory from the
test harness (call t.TempDir() and assign that value to params.SourcePath) so
the runtime.CreateParams in this test uses a valid, platform-safe temp path;
ensure this change is made where the params variable is defined
(runtime.CreateParams) inside the test function that has the *testing.T
parameter.
In `@pkg/runtime/podman/remove_test.go`:
- Around line 211-215: The test case in remove_test.go asserting that
isNotFoundError returns true for the error "failed to inspect container:
permission denied" is likely wrong — update the test to expect false for the
case with err fmt.Errorf("failed to inspect container: permission denied") (or,
if the code intentionally treats all "failed to inspect container" errors as
not-found, change the implementation of isNotFoundError instead and add a
comment and an explicit test case documenting that behavior); locate the test
case by the name "failed to inspect container with other error" and adjust the
expected boolean (or adjust isNotFoundError's logic to explicitly detect
not-found vs permission errors and add corresponding tests).
In `@pkg/runtime/podman/remove.go`:
- Around line 65-70: The podman not-found detector in the function that checks
errMsg is too broad because matching "failed to inspect container" will also
catch permission errors; change the matcher in the not-found helper (the
function containing the strings.Contains checks) to only treat "failed to
inspect container" as not-found when it explicitly includes a not-found phrase
(e.g., match "failed to inspect container: no such" or otherwise require "no
such" in the same message) instead of the current broad contains check, and
update the corresponding test case that expects true for a permission-denied
error (the test at the lines referencing the permission-denied message) to
expect false so permission errors are not classified as not-found.
---
Nitpick comments:
In `@pkg/runtime/podman/create_test.go`:
- Around line 481-484: The two os.MkdirAll calls that create currentDir and
mainDir do not check for errors; change each to capture the returned error (err
:= os.MkdirAll(...)) and fail the test on error (e.g., if err != nil {
t.Fatalf("failed to create %s: %v", currentDir, err) } and similarly for
mainDir) so failures surface in the test; reference the existing os.MkdirAll
calls and the currentDir/mainDir variables in your change.
- Around line 374-376: The three os.MkdirAll calls for currentDir, mainDir, and
sharedDir are not checking errors; update the test to capture and handle each
error (e.g., if err := os.MkdirAll(currentDir, 0755); err != nil {
t.Fatalf("mkdir %s: %v", currentDir, err) } or use require.NoError(t, err) if
testify is available) so any failure to create those directories fails the test
immediately with a helpful message; apply the same pattern to mainDir and
sharedDir.
In `@pkg/runtime/podman/create.go`:
- Line 97: The sudoers file is being written with world-readable permissions via
os.WriteFile(sudoersPath, []byte(sudoersContent), 0644); update the file mode to
a restrictive one (e.g., 0440 or 0400) when writing to sudoersPath to prevent
non-root users from reading it; locate the os.WriteFile call that uses
sudoersPath/sudoersContent in create.go and replace the mode value accordingly.
In `@pkg/runtime/podman/exec/exec.go`:
- Around line 46-55: The Run and Output methods on executor currently drop
podman's stderr, losing useful error details; change Output to use
cmd.CombinedOutput() and return that output (so stderr is included) and change
Run to capture combined output (e.g., via cmd.CombinedOutput() or by setting
cmd.Stderr to a buffer) and, on error, wrap/return an error that includes the
combined stdout+stderr text; update functions executor.Run and executor.Output
to surface podman stderr in returned errors or outputs for better diagnostics.
In `@pkg/runtime/podman/info.go`:
- Around line 44-54: The inspect call currently uses p.executor.Output and
splits on "|" (see use of p.executor.Output(... "inspect" ... id) and the fields
variable), which fails to distinguish a missing container and is brittle for
delimiters; change to request JSON from podman (use --format '{{json .}}'),
unmarshal into a small struct containing Id, State.Status, and ImageName, and
build the runtime.RuntimeInfo from those fields instead of splitting; detect
podman’s "no such container" error text from the command error and wrap/return
runtime.ErrInstanceNotFound for that case (where the existing code returns
fmt.Errorf("failed to inspect container: %w", err)), and remove the brittle
string split and len(fields) check.
In `@pkg/runtime/podman/podman_test.go`:
- Around line 97-109: The Getuid/Getgid helpers treat a zero value as "unset"
and return 1000, which prevents simulating UID/GID 0 (root); update the
fakeSystem struct to distinguish "not set" from zero (e.g., change uid and gid
to *int or add explicit bool flags like uidSet/gidSet), then modify Getuid and
Getgid to check for nil or the flags instead of value==0 and return the stored 0
when explicitly set; also update any test constructors/initializers for
fakeSystem to set the pointer or flags when intending to simulate specific
UIDs/GIDs.
In `@pkg/runtime/podman/remove_test.go`:
- Around line 235-247: Replace the custom contains and stringContains helpers
with the standard library strings.Contains: remove the functions contains and
stringContains, add "strings" to the imports, and update any call sites that use
contains (e.g., the test that currently calls contains) to call
strings.Contains(s, substr) instead.
In `@pkg/system/system_test.go`:
- Around line 35-47: The two subtests created with t.Run in system_test.go
("existing command" and "non-existing command") are missing t.Parallel(); add
t.Parallel() as the first statement inside each subtest's func(t *testing.T)
body so both subtests run in parallel (i.e., inside the funcs that call
sys.CommandExists, call t.Parallel() immediately before any assertions).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e453a05f-398d-45c2-8f06-3434207e2b16
📒 Files selected for processing (20)
README.mdpkg/instances/manager.gopkg/instances/manager_test.gopkg/runtime/podman/create.gopkg/runtime/podman/create_test.gopkg/runtime/podman/exec/exec.gopkg/runtime/podman/exec/fake.gopkg/runtime/podman/info.gopkg/runtime/podman/info_test.gopkg/runtime/podman/podman.gopkg/runtime/podman/podman_test.gopkg/runtime/podman/remove.gopkg/runtime/podman/remove_test.gopkg/runtime/podman/start.gopkg/runtime/podman/start_test.gopkg/runtime/podman/stop.gopkg/runtime/podman/stop_test.gopkg/runtimesetup/register.gopkg/system/system.gopkg/system/system_test.go
Contributor
|
Tried it on Windows but got error:
Seems to be cause by os.Getuid and os.Getgid returning -1 on Windows |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/runtime/podman/create.go (1)
240-266: Consider cleanup on failure to avoid orphaned instance directories.If
createContainerfile,buildImage, orcreateContainerfails after the instance directory is created, the directory will remain on disk. Over time, this could accumulate orphaned directories.♻️ Proposed fix using defer for cleanup
// Create instance directory instanceDir, err := p.createInstanceDirectory(params.Name) if err != nil { return runtime.RuntimeInfo{}, err } + // Track success to avoid cleanup on successful completion + success := false + defer func() { + if !success { + _ = os.RemoveAll(instanceDir) + } + }() // Create Containerfile if err := p.createContainerfile(instanceDir); err != nil { return runtime.RuntimeInfo{}, err }Then set
success = truejust before the successful return:+ success = true return runtime.RuntimeInfo{ ID: containerID, State: "created", Info: info, }, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/podman/create.go` around lines 240 - 266, The instance directory created by p.createInstanceDirectory(params.Name) is never removed on subsequent failures, so wrap the directory creation with a cleanup guard: after calling createInstanceDirectory and getting instanceDir, set a deferred cleanup that removes instanceDir unless a local success flag is set to true; mark success = true immediately before the routine returns successfully (after p.createContainer completes). Ensure the defer references instanceDir and runs only when not successful, and leave the normal control flow for createContainerfile, buildImage, buildContainerArgs, and createContainer unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/runtime/podman/create.go`:
- Around line 97-99: The sudoers file is currently created with permissive mode
0644 in the os.WriteFile call; update the write to use restrictive mode 0440 so
the sudoers file is not world-writable/readable (locate the
os.WriteFile(sudoersPath, []byte(sudoersContent), 0644) call in
pkg/runtime/podman/create.go and change the file mode argument to 0440),
ensuring the sudoersPath file is created with proper permissions.
In `@pkg/system/system_windows.go`:
- Around line 27-37: The WSL calls in systemImpl.Getuid (and the sibling Getgid)
can block indefinitely; create a small helper (e.g., runWSLCommandWithTimeout or
wslOutputWithTimeout) that uses context.WithTimeout and exec.CommandContext to
run "wsl ..." with a short timeout (e.g., a few seconds), use that helper from
Getuid (and Getgid) to get output, parse the trimmed output as before, and on
timeout/error return the existing fallback (1000) so resolution never blocks the
caller.
---
Nitpick comments:
In `@pkg/runtime/podman/create.go`:
- Around line 240-266: The instance directory created by
p.createInstanceDirectory(params.Name) is never removed on subsequent failures,
so wrap the directory creation with a cleanup guard: after calling
createInstanceDirectory and getting instanceDir, set a deferred cleanup that
removes instanceDir unless a local success flag is set to true; mark success =
true immediately before the routine returns successfully (after
p.createContainer completes). Ensure the defer references instanceDir and runs
only when not successful, and leave the normal control flow for
createContainerfile, buildImage, buildContainerArgs, and createContainer
unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 827e2c14-89ce-452a-8a7a-98ea790df55e
📒 Files selected for processing (5)
pkg/runtime/podman/create.gopkg/system/system.gopkg/system/system_test.gopkg/system/system_unix.gopkg/system/system_windows.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/system/system_test.go
- pkg/system/system.go
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com> Co-Authored-By: Claude Code (Claude Sonnet 4.5) <noreply@anthropic.com>
feloy
force-pushed
the
podman-runtime-branch
branch
from
b35cbe2 to
6e21e2b
Compare
Contributor
Author
|
@jeffmaury I have some changes to support UID/GID on Windows, and tested on my Windows machine, it should work better now |
Contributor
Tested on Windows/WSL, works for me. Only question I have is how to select the Podman machine. |
jeffmaury
reviewed
Mar 20, 2026
pkg/runtime/podman/exec/fake.go
Outdated
|
|
||
| // CommandString returns a string representation of a command for debugging. | ||
| func CommandString(args ...string) string { | ||
| return "podman " + strings.Join(args, " ") |
Contributor
There was a problem hiding this comment.
thought: might be confusing in case of argument with spaces
Contributor
Author
There was a problem hiding this comment.
It appears that this code which was introduced for tests is not used anymorE. I removed it
| // On Windows, this queries WSL2 for the actual UID and falls back to 1000 if unavailable. | ||
| func (s *systemImpl) Getuid() int { | ||
| // Try to get UID from WSL2 | ||
| cmd := exec.Command("wsl", "id", "-u") |
Contributor
There was a problem hiding this comment.
praise: seems wrong to me as it will launch the id command in the default WSL distribution which is not guaranted to be the Podman machine
Contributor
Author
There was a problem hiding this comment.
USER=$(podman machine inspect --format "{{ .SSHConfig.RemoteUsername }}")
ID=$(podman machine ssh --username $USER id -u)
This is working for me on both macOS and on Windows/WSL.
@benoitf do you think this is something we can rely on for macOS and Windows?
Contributor
There was a problem hiding this comment.
Using $USER is your host user name which may not exist in the Podman machine (root or user)
Contributor
Author
There was a problem hiding this comment.
No here USER is extracted from 'machine inspect' Remote username
Contributor
Author
For now it is using the default one. I made a quick test, defining the CONTAINER_CONNECTION variable should be taken into consideration for using another machine: |
Signed-off-by: Philippe Martin <phmartin@redhat.com>
feloy
force-pushed
the
podman-runtime-branch
branch
from
5f52632 to
be26a72
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/runtime/podman/create.go`:
- Around line 58-64: validateCreateParams currently allows path-like names that
can escape p.storageDir or produce invalid Podman identifiers; modify
validateCreateParams to reject any params.Name that contains path separators or
traversal segments (e.g. "/" or "\" or "..") and only accept a safe identifier
(for example enforce that params.Name == filepath.Base(params.Name) and does not
contain "/" or "\" and does not equal "." or "..", or validate against a regex
allowing only alphanumerics, hyphen and underscore). Update the check in
validateCreateParams (function name: validateCreateParams, type:
runtime.CreateParams) to return runtime.ErrInvalidParams when the name is
invalid and include a short message explaining the name restriction.
- Around line 167-199: The host-side mount paths are not made absolute before
building the "-v" args; update the logic in the pod/container mount loop that
handles params.SourcePath, WorkspaceConfig.Mounts.Dependencies and
WorkspaceConfig.Mounts.Configs to call filepath.Abs() for the host path (e.g.
compute depAbsPath via filepath.Abs(filepath.Join(params.SourcePath, depOSPath))
and confAbsPath via filepath.Abs(filepath.Join(homeDir, confOSPath))) and return
an error if filepath.Abs fails; additionally for configs verify the resolved
confAbsPath is inside homeDir (reject if it escapes the user home) before
constructing confMountPoint and appending the "-v" args so container mounts
always use absolute, validated host paths.
- Around line 103-113: The package list in the initial RUN that installs
required packages (the line starting with RUN dnf install -y which procps-ng
wget2 ...) is missing curl, but later the script pipes
https://claude.ai/install.sh to bash using curl; update that RUN command to
include curl in the installed packages so the container has curl available at
runtime (modify the RUN dnf install -y ... invocation that includes which,
procps-ng, wget2, `@development-tools`, jq, gh, golang, golangci-lint, python3,
python3-pip to also install curl).
- Around line 217-230: Change createContainer to capture and return the
container ID directly from the podman create stdout instead of discarding output
and calling getContainerID; specifically replace p.executor.Run in
createContainer with p.executor.Output (or equivalent) to read the created
container ID, trim and return it to callers (adjust signature from
createContainer(ctx, args []string) error to return (string, error)), and remove
or deprecate the separate getContainerID usage so callers no longer call inspect
after create; if you prefer not to change callers, alternatively ensure
createContainer performs a cleanup (podman rm) on error after create succeeds
but inspect fails so a partially-created container is removed before returning
an error.
In `@pkg/runtime/podman/exec/fake.go`:
- Around line 22-45: Rename the exported FakeExecutor struct to an unexported
fakeExecutor and make NewFake return the public Executor interface (not
*FakeExecutor); update the compile-time assertion to var _ Executor =
(*fakeExecutor)(nil) and adjust any references to the concrete type accordingly
so callers only see the interface from NewFake while the implementation and its
fields (RunFunc, OutputFunc, RunCalls, OutputCalls) remain unexported in pkg.
- Around line 49-58: The test double mutates and stores the original args slice
by appending args directly to RunCalls/OutputCalls, risk of later external
mutation; update FakeExecutor.Run and FakeExecutor.Output to snapshot the
variadic args before storing (e.g., create a new slice copy via
append([]string(nil), args...) or make([]string, len(args)) and copy) and append
that copy to RunCalls/OutputCalls, then invoke RunFunc/OutputFunc using the
original args as before.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80ad909c-3235-491b-bd71-e4481f09fe90
📒 Files selected for processing (6)
pkg/runtime/podman/create.gopkg/runtime/podman/exec/fake.gopkg/system/system.gopkg/system/system_test.gopkg/system/system_unix.gopkg/system/system_windows.go
✅ Files skipped from review due to trivial changes (1)
- pkg/system/system_unix.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/system/system_test.go
- pkg/system/system_windows.go
Comment on lines
+58
to
+64
| func (p *podmanRuntime) validateCreateParams(params runtime.CreateParams) error { | ||
| if params.Name == "" { | ||
| return fmt.Errorf("%w: name is required", runtime.ErrInvalidParams) | ||
| } | ||
| if params.SourcePath == "" { | ||
| return fmt.Errorf("%w: source path is required", runtime.ErrInvalidParams) | ||
| } |
There was a problem hiding this comment.
Reject path-like instance names up front.
params.Name is later joined into p.storageDir/instances/<name> and reused in the Podman image/container identifiers. Values like ../foo or a/b can escape the instances directory or create invalid Podman names, so validateCreateParams should reject separators and traversal segments here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runtime/podman/create.go` around lines 58 - 64, validateCreateParams
currently allows path-like names that can escape p.storageDir or produce invalid
Podman identifiers; modify validateCreateParams to reject any params.Name that
contains path separators or traversal segments (e.g. "/" or "\" or "..") and
only accept a safe identifier (for example enforce that params.Name ==
filepath.Base(params.Name) and does not contain "/" or "\" and does not equal
"." or "..", or validate against a regex allowing only alphanumerics, hyphen and
underscore). Update the check in validateCreateParams (function name:
validateCreateParams, type: runtime.CreateParams) to return
runtime.ErrInvalidParams when the name is invalid and include a short message
explaining the name restriction.
Comment on lines
+103
to
+113
| RUN dnf install -y which procps-ng wget2 @development-tools jq gh golang golangci-lint python3 python3-pip | ||
|
|
||
| ARG UID=1000 | ||
| ARG GID=1000 | ||
| RUN GROUPNAME=$(grep $GID /etc/group | cut -d: -f1); [ -n "$GROUPNAME" ] && groupdel $GROUPNAME || true | ||
| RUN groupadd -g "${GID}" claude && useradd -u "${UID}" -g "${GID}" -m claude | ||
| COPY sudoers /etc/sudoers.d/claude | ||
| USER claude:claude | ||
|
|
||
| ENV PATH=/home/claude/.local/bin:/usr/local/bin:/usr/bin | ||
| RUN curl -fsSL --proto-redir '-all,https' --tlsv1.3 https://claude.ai/install.sh | bash |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n pkg/runtime/podman/create.go | sed -n '95,120p'Repository: kortex-hub/kortex-cli
Length of output: 1295
🌐 Web query:
Fedora latest base image default packages sudo curl included
💡 Result:
No, the latest Fedora base container image (fedora:latest, currently Fedora 42 as of 2026-03-20) does not include curl by default. Sudo is included. The official Fedora Docker image is built using a kickstart file from fedora-kickstarts that explicitly installs: bash, coreutils, glibc-minimal-langpack, rpm, util-linux-core, rootfiles, tar, vim-minimal, dnf5 (or dnf), dnf5-plugins (or dnf-plugins-core), sudo. Curl is not listed in the kickstart or osbuild container image descriptions. Osbuild image builder docs for Fedora 42 container type confirm os packages: bash, coreutils, dnf, fedora-release-container, glibc-minimal-langpack, rootfiles, rpm, sudo, tar, util-linux-core, vim-minimal, yum. No curl. Toolbox variant includes curl, but standard base does not.
Citations:
- 1: https://hub.docker.com/_/fedora
- 2: https://osbuild.org/docs/user-guide/image-descriptions/fedora-41/container
- 3: https://osbuild.org/docs/user-guide/image-descriptions/fedora-42/container
- 4: https://pagure.io/fedora-kickstarts/blob/main/f/fedora-container-base.ks
- 5: https://pagure.io/fedora-kickstarts/tree/master
🏁 Script executed:
head -20 pkg/runtime/podman/create.goRepository: kortex-hub/kortex-cli
Length of output: 704
Add curl to the dnf install command.
Line 113 uses curl to bootstrap the environment, but it is not included in the Fedora base image's default packages. The Containerfile will fail at runtime if curl is not explicitly installed. Sudo is already included in the base image, so it does not need to be added.
🔧 Suggested change
-RUN dnf install -y which procps-ng wget2 `@development-tools` jq gh golang golangci-lint python3 python3-pip
+RUN dnf install -y curl which procps-ng wget2 `@development-tools` jq gh golang golangci-lint python3 python3-pip📝 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.
Suggested change
| RUN dnf install -y which procps-ng wget2 @development-tools jq gh golang golangci-lint python3 python3-pip | |
| ARG UID=1000 | |
| ARG GID=1000 | |
| RUN GROUPNAME=$(grep $GID /etc/group | cut -d: -f1); [ -n "$GROUPNAME" ] && groupdel $GROUPNAME || true | |
| RUN groupadd -g "${GID}" claude && useradd -u "${UID}" -g "${GID}" -m claude | |
| COPY sudoers /etc/sudoers.d/claude | |
| USER claude:claude | |
| ENV PATH=/home/claude/.local/bin:/usr/local/bin:/usr/bin | |
| RUN curl -fsSL --proto-redir '-all,https' --tlsv1.3 https://claude.ai/install.sh | bash | |
| RUN dnf install -y curl which procps-ng wget2 `@development-tools` jq gh golang golangci-lint python3 python3-pip | |
| ARG UID=1000 | |
| ARG GID=1000 | |
| RUN GROUPNAME=$(grep $GID /etc/group | cut -d: -f1); [ -n "$GROUPNAME" ] && groupdel $GROUPNAME || true | |
| RUN groupadd -g "${GID}" claude && useradd -u "${UID}" -g "${GID}" -m claude | |
| COPY sudoers /etc/sudoers.d/claude | |
| USER claude:claude | |
| ENV PATH=/home/claude/.local/bin:/usr/local/bin:/usr/bin | |
| RUN curl -fsSL --proto-redir '-all,https' --tlsv1.3 https://claude.ai/install.sh | bash |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runtime/podman/create.go` around lines 103 - 113, The package list in the
initial RUN that installs required packages (the line starting with RUN dnf
install -y which procps-ng wget2 ...) is missing curl, but later the script
pipes https://claude.ai/install.sh to bash using curl; update that RUN command
to include curl in the installed packages so the container has curl available at
runtime (modify the RUN dnf install -y ... invocation that includes which,
procps-ng, wget2, `@development-tools`, jq, gh, golang, golangci-lint, python3,
python3-pip to also install curl).
Comment on lines
+167
to
+199
| args = append(args, "-v", fmt.Sprintf("%s:/workspace/sources:Z", params.SourcePath)) | ||
|
|
||
| // Mount dependencies if specified | ||
| // Dependencies are mounted at /workspace/<dep-path> to preserve relative paths for symlinks | ||
| // Example: if source has a symlink "../main/file", it will resolve to /workspace/main/file | ||
| if params.WorkspaceConfig != nil && params.WorkspaceConfig.Mounts != nil { | ||
| if params.WorkspaceConfig.Mounts.Dependencies != nil { | ||
| for _, dep := range *params.WorkspaceConfig.Mounts.Dependencies { | ||
| // Convert dependency path from forward slashes (cross-platform config format) | ||
| // to OS-specific separators for the host path | ||
| depOSPath := filepath.FromSlash(dep) | ||
| depAbsPath := filepath.Join(params.SourcePath, depOSPath) | ||
| // Mount at /workspace/sources/<dep-path> to preserve relative path structure | ||
| // Use path.Join (not filepath.Join) for container paths to ensure forward slashes | ||
| depMountPoint := path.Join("/workspace/sources", dep) | ||
| args = append(args, "-v", fmt.Sprintf("%s:%s:Z", depAbsPath, depMountPoint)) | ||
| } | ||
| } | ||
|
|
||
| // Mount configs if specified | ||
| if params.WorkspaceConfig.Mounts.Configs != nil { | ||
| homeDir, err := os.UserHomeDir() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get home directory: %w", err) | ||
| } | ||
| for _, conf := range *params.WorkspaceConfig.Mounts.Configs { | ||
| // Convert config path from forward slashes to OS-specific separators | ||
| confOSPath := filepath.FromSlash(conf) | ||
| confAbsPath := filepath.Join(homeDir, confOSPath) | ||
| // HOME in container is /home/claude for the image, this may be different for other images | ||
| // Use path.Join for container paths to ensure forward slashes | ||
| confMountPoint := path.Join("/home/claude", conf) | ||
| args = append(args, "-v", fmt.Sprintf("%s:%s:Z", confAbsPath, confMountPoint)) |
There was a problem hiding this comment.
Resolve host bind paths to absolute paths before building -v args.
params.SourcePath, the dependency joins, and the config joins are only passed through Join, so relative inputs stay relative and Podman resolves them against the caller’s working directory. For configs, values like ../../.ssh also clean outside homeDir. Resolve the host side with filepath.Abs() and reject any config whose resolved path escapes the user home before appending it. As per coding guidelines, Convert relative paths to absolute with 'filepath.Abs()' when handling user-provided paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runtime/podman/create.go` around lines 167 - 199, The host-side mount
paths are not made absolute before building the "-v" args; update the logic in
the pod/container mount loop that handles params.SourcePath,
WorkspaceConfig.Mounts.Dependencies and WorkspaceConfig.Mounts.Configs to call
filepath.Abs() for the host path (e.g. compute depAbsPath via
filepath.Abs(filepath.Join(params.SourcePath, depOSPath)) and confAbsPath via
filepath.Abs(filepath.Join(homeDir, confOSPath))) and return an error if
filepath.Abs fails; additionally for configs verify the resolved confAbsPath is
inside homeDir (reject if it escapes the user home) before constructing
confMountPoint and appending the "-v" args so container mounts always use
absolute, validated host paths.
Comment on lines
+217
to
+230
| func (p *podmanRuntime) createContainer(ctx context.Context, args []string) error { | ||
| if err := p.executor.Run(ctx, args...); err != nil { | ||
| return fmt.Errorf("failed to create podman container: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // getContainerID retrieves the container ID for a given container name. | ||
| func (p *podmanRuntime) getContainerID(ctx context.Context, name string) (string, error) { | ||
| output, err := p.executor.Output(ctx, "inspect", "--format", "{{.Id}}", name) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to inspect container: %w", err) | ||
| } | ||
| return strings.TrimSpace(string(output)), nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -n 300 pkg/runtime/podman/create.go | tail -n +200Repository: kortex-hub/kortex-cli
Length of output: 2388
🏁 Script executed:
# Find the runtime interface definition
rg "interface.*Runtime" pkg/runtime/ -A 10
# Check if createContainer and getContainerID are used elsewhere
rg "createContainer|getContainerID" pkg/runtime/podman/Repository: kortex-hub/kortex-cli
Length of output: 1114
🏁 Script executed:
# Find the runtime interface definition
find pkg/runtime -name "*.go" -type f | xargs grep -l "type Runtime interface\|type.*interface"
# Check the interface more specifically
rg "type.*Runtime.*interface" pkg/ -A 15
# Check what executor.Run and executor.Output do
rg "type.*Executor|interface.*Executor" pkg/ -A 10Repository: kortex-hub/kortex-cli
Length of output: 2750
🏁 Script executed:
# Check if there's any documentation or comments about podman create output
rg "podman create" pkg/ --type go -B 2 -A 2
# Check the full Create function to see error handling and cleanup paths
sed -n '245,290p' pkg/runtime/podman/create.go
# Look for any teardown or cleanup on error
rg "defer|cleanup|defer.*remove|defer.*delete" pkg/runtime/podman/ --type goRepository: kortex-hub/kortex-cli
Length of output: 1028
🏁 Script executed:
# Check what buildContainerArgs produces - see the full function
rg "func.*buildContainerArgs" pkg/runtime/podman/ -A 50
# Check if podman create outputs container ID by looking for any format flags
rg "buildContainerArgs\|createArgs" pkg/runtime/podman/create.go -B 2 -A 5Repository: kortex-hub/kortex-cli
Length of output: 4127
🏁 Script executed:
# Get the complete buildContainerArgs function
sed -n '120,220p' pkg/runtime/podman/create.go
# Check if there's any usage of --cidfile or similar that captures container ID
rg "cidfile\|--format.*Id" pkg/runtime/podman/Repository: kortex-hub/kortex-cli
Length of output: 4039
🏁 Script executed:
# Check if there are any tests that show what podman create outputs
rg "create" pkg/runtime/podman/create_test.go -B 2 -A 5 | head -100
# Check what the buildContainerArgs output looks like (what args are passed to podman)
sed -n '145,230p' pkg/runtime/podman/create.go
# Check if there's documentation or comments about podman output
rg "stdout\|output\|returns" pkg/runtime/podman/ --type go -B 2 -A 2 | head -50Repository: kortex-hub/kortex-cli
Length of output: 6166
🏁 Script executed:
# Let me check what podman create outputs by default
# First, see if there's any documentation or examples in the codebase
rg "podman.*create" . --type md --type txt
# Check if there's any error handling or cleanup code
sed -n '230,280p' pkg/runtime/podman/create.go
# Also check if getContainerID is called anywhere else
rg "getContainerID" pkg/Repository: kortex-hub/kortex-cli
Length of output: 1716
🏁 Script executed:
# Verify that podman create outputs container ID by checking if there's any mention of this behavior
web_search "podman create command output container id"Repository: kortex-hub/kortex-cli
Length of output: 115
🌐 Web query:
podman create command output container id
💡 Result:
The podman create command outputs the container ID to STDOUT upon successful creation. This is explicitly stated in the official Podman documentation: "The container ID is then printed to STDOUT." This behavior is consistent across multiple versions of the documentation (v5.0.2, v5.6.0, and stable). For example, running podman create alpine echo hello will print a container ID like cc31f5a3cc95 (short ID) or the full ID, which can then be captured in a variable, e.g., ctr=$(podman create alpine echo hello). The container is created in a 'created' state and must be started separately with podman start $ctr.
Citations:
- 1: https://docs.podman.io/en/v5.0.2/markdown/podman-create.1.html
- 2: https://docs.podman.io/en/v5.6.0/markdown/podman-create.1.html
- 3: https://docs.podman.io/en/stable/markdown/podman-create.1.html
Eliminate the extra Podman round-trip to fetch the container ID.
The current implementation creates a container but discards its output, then makes a separate podman inspect call to retrieve the ID. If inspect fails after create succeeds, the container exists but Create() returns an error, blocking retries by name since the name is already taken.
Instead, capture the container ID directly from podman create's stdout by changing createContainer() to use executor.Output() and return the ID. This eliminates the second round-trip and the failure scenario. Alternatively, add cleanup to remove the container before returning an error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runtime/podman/create.go` around lines 217 - 230, Change createContainer
to capture and return the container ID directly from the podman create stdout
instead of discarding output and calling getContainerID; specifically replace
p.executor.Run in createContainer with p.executor.Output (or equivalent) to read
the created container ID, trim and return it to callers (adjust signature from
createContainer(ctx, args []string) error to return (string, error)), and remove
or deprecate the separate getContainerID usage so callers no longer call inspect
after create; if you prefer not to change callers, alternatively ensure
createContainer performs a cleanup (podman rm) on error after create succeeds
but inspect fails so a partially-created container is removed before returning
an error.
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
3 participants
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.
Implements the podman runtime:
Document the runtime on README
Fixes #75
Container configuration will be done with #84