From c31f0e9c71ba089d74107a384a87cabb88c8cd08 Mon Sep 17 00:00:00 2001 From: Mirko Brombin Date: Thu, 2 Apr 2026 12:53:32 +0200 Subject: [PATCH] fix: various bug fixes and code improvements (#2047) * fix(containermanager): fix typos in ContanerID and ContanerManagerType * fix(rm): pass ContainerHome to RmOptions when removing container * fix(containermanager): handle empty names slice in podman container list * fix(create): initialize prompter in NewCreateCommand to prevent nil dereference * fix(userenv): trim trailing newline from uid and gid command output * refactor(containermanager): move shared types and constants to providers common file * fix(containermanager): skip /dev/shm volume mount if EvalSymlinks fails * fix(containermanager): rename ContainerManagerType to ManagerType to avoid stutter * restore: ManagerType -> ContainerManagerType * test(containermanager): add empty names fallback test for parsePodmanContainerList * fix(containermanager): suppress revive stutter lint for ContainerManagerType * fix(containermanager): use empty string fallback when podman container has no name * fix(commands): pass real prompter to create command in assemble and ephemeral * Revert "refactor(containermanager): move shared types and constants to providers common file" This reverts commit 59faeb71f863b62c1629ed7b257fa910107329a7. --- internal/cli/create.go | 4 +- internal/cli/ephemeral.go | 4 +- internal/userenv/user_environment.go | 4 +- pkg/commands/assemble.go | 2 +- pkg/commands/create.go | 7 +-- pkg/commands/ephemeral.go | 5 +- pkg/commands/rm.go | 5 +- pkg/containermanager/containermanager.go | 5 +- pkg/containermanager/providers/docker.go | 8 ++-- pkg/containermanager/providers/podman.go | 13 ++++-- .../providers/podman_internal_test.go | 46 +++++++++++++++++++ 11 files changed, 83 insertions(+), 20 deletions(-) diff --git a/internal/cli/create.go b/internal/cli/create.go index 16242572..97841325 100644 --- a/internal/cli/create.go +++ b/internal/cli/create.go @@ -1,6 +1,7 @@ package cli import ( + "bufio" "context" "errors" "fmt" @@ -213,8 +214,9 @@ func createAction(ctx context.Context, cmd *cli.Command) error { } progress := ui.NewProgress(os.Stderr) + prompter := ui.NewPrompter(*bufio.NewReader(os.Stdin), os.Stdout) - createCmd := commands.NewCreateCommand(containerManager, progress) + createCmd := commands.NewCreateCommand(containerManager, progress, prompter) err := createCmd.Execute(ctx, opts) var containerAlreadyExistsErr *commands.ContainerAlreadyExistsError diff --git a/internal/cli/ephemeral.go b/internal/cli/ephemeral.go index 7b47438b..c3b68841 100644 --- a/internal/cli/ephemeral.go +++ b/internal/cli/ephemeral.go @@ -1,6 +1,7 @@ package cli import ( + "bufio" "context" "errors" "fmt" @@ -78,8 +79,9 @@ func ephemeralAction(ctx context.Context, cmd *cli.Command) error { progress := ui.NewProgress(os.Stderr) printer := ui.NewPrinter(os.Stderr, true) + prompter := ui.NewPrompter(*bufio.NewReader(os.Stdin), os.Stdout) - ephemeralCmd := commands.NewEphemeralCommand(containerManager, progress, printer) + ephemeralCmd := commands.NewEphemeralCommand(containerManager, progress, printer, prompter) err := ephemeralCmd.Execute(ctx, opts) if err != nil { diff --git a/internal/userenv/user_environment.go b/internal/userenv/user_environment.go index 8893af63..9970c4e9 100644 --- a/internal/userenv/user_environment.go +++ b/internal/userenv/user_environment.go @@ -71,14 +71,14 @@ func LoadUserEnvironment(ctx context.Context) *UserEnvironment { if uid := os.Getuid(); uid >= 0 { env.UserID = strconv.Itoa(uid) } else if uid, err := exec.CommandContext(ctx, "id", "-ru").Output(); err == nil { - env.UserID = string(uid) + env.UserID = strings.TrimSpace(string(uid)) } // GROUP ID if gid := os.Getgid(); gid >= 0 { env.GroupID = strconv.Itoa(gid) } else if gid, err := exec.CommandContext(ctx, "id", "-rg").Output(); err == nil { - env.GroupID = string(gid) + env.GroupID = strings.TrimSpace(string(gid)) } return env diff --git a/pkg/commands/assemble.go b/pkg/commands/assemble.go index 45d5a733..f6b6a72b 100644 --- a/pkg/commands/assemble.go +++ b/pkg/commands/assemble.go @@ -42,7 +42,7 @@ func NewAssembleCommand( ) *AssembleCommand { return &AssembleCommand{ containermanager: cm, - createCmd: NewCreateCommand(cm, ui.NewDevNullProgress()), + createCmd: NewCreateCommand(cm, ui.NewDevNullProgress(), prompter), rmCmd: NewRmCommand(cm, prompter), enterCmd: NewEnterCommand(cm, progress, printer), progress: progress, diff --git a/pkg/commands/create.go b/pkg/commands/create.go index fa63f2e8..ed242fce 100644 --- a/pkg/commands/create.go +++ b/pkg/commands/create.go @@ -84,11 +84,12 @@ type CreateOptions struct { NonInteractive bool } -func NewCreateCommand(cm containermanager.ContainerManager, progress *ui.Progress) *CreateCommand { +func NewCreateCommand(cm containermanager.ContainerManager, progress *ui.Progress, prompter *ui.Prompter) *CreateCommand { return &CreateCommand{ containerManager: cm, generateEntryCmd: NewGenerateEntryCommand(NewListCommand(cm)), progress: progress, + prompter: prompter, } } @@ -267,9 +268,9 @@ func (c *CreateCommand) clone(ctx context.Context, containerName string) (string commitTag := fmt.Sprintf("%s:%s", strings.ToLower(containerName), time.Now().Format("2006-01-02")) - err = c.containerManager.Commit(ctx, i.ContanerID, commitTag) + err = c.containerManager.Commit(ctx, i.ContainerID, commitTag) if err != nil { - return "", fmt.Errorf("failed to commit container '%s:%s': %w", i.ContanerID, commitTag, err) + return "", fmt.Errorf("failed to commit container '%s:%s': %w", i.ContainerID, commitTag, err) } return commitTag, nil diff --git a/pkg/commands/ephemeral.go b/pkg/commands/ephemeral.go index 8bac38e2..aa858b03 100644 --- a/pkg/commands/ephemeral.go +++ b/pkg/commands/ephemeral.go @@ -26,12 +26,13 @@ func NewEphemeralCommand( cm containermanager.ContainerManager, progress *ui.Progress, printer *ui.Printer, + prompter *ui.Prompter, ) *EphemeralCommand { return &EphemeralCommand{ containerManager: cm, - createCmd: NewCreateCommand(cm, progress), + createCmd: NewCreateCommand(cm, progress, prompter), enterCmd: NewEnterCommand(cm, progress, printer), - rmCmd: NewRmCommand(cm, nil), + rmCmd: NewRmCommand(cm, prompter), } } diff --git a/pkg/commands/rm.go b/pkg/commands/rm.go index 3fa41d14..d6c6122e 100644 --- a/pkg/commands/rm.go +++ b/pkg/commands/rm.go @@ -108,8 +108,9 @@ func (c *RmCommand) removeContainer( } cmOptions := containermanager.RmOptions{ - Force: forceRemove, - RemoveHome: removeHome, + Force: forceRemove, + RemoveHome: removeHome, + ContainerHome: inspectOutput.ContainerHome, } err = c.containerManager.Remove(ctx, container.Name, cmOptions) if err != nil { diff --git a/pkg/containermanager/containermanager.go b/pkg/containermanager/containermanager.go index 5981e3c8..e2a8ff30 100644 --- a/pkg/containermanager/containermanager.go +++ b/pkg/containermanager/containermanager.go @@ -16,7 +16,7 @@ type Container struct { } type InspectResult struct { - ContanerID string + ContainerID string ContainerStatus string ContainerHome string ContainerPath string @@ -73,7 +73,8 @@ func (c Container) IsRunning() bool { return strings.Contains(s, "up") || strings.Contains(s, "running") } -type ContanerManagerType string +//nolint:revive // ContainerManagerType is intentionally named for clarity despite the stutter +type ContainerManagerType string type ContainerManager interface { Name() string diff --git a/pkg/containermanager/providers/docker.go b/pkg/containermanager/providers/docker.go index 5e1e6bf2..5ae7afe1 100644 --- a/pkg/containermanager/providers/docker.go +++ b/pkg/containermanager/providers/docker.go @@ -305,8 +305,10 @@ func (d *Docker) makeCreateCommand( // Resolve this detecting if /dev/shm is a symlink and mount original // source also in the container. if isSymlink("/dev/shm") && !unshareIPC { - realPath, _ := filepath.EvalSymlinks("/dev/shm") - options = append(options, "--volume", fmt.Sprintf("%s:%s", realPath, realPath)) + realPath, err := filepath.EvalSymlinks("/dev/shm") + if err == nil { + options = append(options, "--volume", fmt.Sprintf("%s:%s", realPath, realPath)) + } } // Ensure support forwarding of RedHat subscription-manager @@ -675,7 +677,7 @@ func (d *Docker) InspectContainer(ctx context.Context, containerName string) (*c } inspect := inspects[0] - config.ContanerID = inspect.ID + config.ContainerID = inspect.ID config.ContainerStatus = inspect.State.Status // Check for unshare_groups label diff --git a/pkg/containermanager/providers/podman.go b/pkg/containermanager/providers/podman.go index 044b903c..5fb5ec70 100644 --- a/pkg/containermanager/providers/podman.go +++ b/pkg/containermanager/providers/podman.go @@ -266,8 +266,10 @@ func (p *Podman) makeCreateCommand( // Resolve this detecting if /dev/shm is a symlink and mount original // source also in the container. if isSymlink("/dev/shm") && !unshareIPC { - realPath, _ := filepath.EvalSymlinks("/dev/shm") - options = append(options, "--volume", fmt.Sprintf("%s:%s", realPath, realPath)) + realPath, err := filepath.EvalSymlinks("/dev/shm") + if err == nil { + options = append(options, "--volume", fmt.Sprintf("%s:%s", realPath, realPath)) + } } // Ensure support forwarding of RedHat subscription-manager @@ -615,10 +617,15 @@ func parsePodmanContainerList(output string) ([]containermanager.Container, erro id = id[:containerIDMaxLength] } + name := "" + if len(c.Names) > 0 { + name = c.Names[0] + } + containers = append(containers, containermanager.Container{ ID: id, Image: c.Image, - Name: c.Names[0], + Name: name, Status: c.Status, Labels: c.Labels, }) diff --git a/pkg/containermanager/providers/podman_internal_test.go b/pkg/containermanager/providers/podman_internal_test.go index 92da0ae6..3d30af8b 100644 --- a/pkg/containermanager/providers/podman_internal_test.go +++ b/pkg/containermanager/providers/podman_internal_test.go @@ -663,6 +663,52 @@ func TestParsePodmanContainerListEmpty(t *testing.T) { } } +func TestParsePodmanContainerListEmptyNames(t *testing.T) { + tests := []struct { + name string + json string + wantName string + wantID string + }{ + { + name: "empty names array falls back to empty string", + json: `[{"ID":"abc123def456789012345678","Image":"fedora:39","Names":[],"Status":"running","Labels":{}}]`, + wantName: "", + wantID: "abc123def456", + }, + { + name: "null names falls back to empty string", + json: `[{"ID":"xyz789abc123456789012345","Image":"ubuntu:22.04","Names":null,"Status":"exited","Labels":{}}]`, + wantName: "", + wantID: "xyz789abc123", + }, + { + name: "short ID (under 12 chars) is used as-is", + json: `[{"ID":"shortid","Image":"alpine:latest","Names":[],"Status":"running","Labels":{}}]`, + wantName: "", + wantID: "shortid", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + containers, err := parsePodmanContainerList(tt.json) + if err != nil { + t.Fatalf("parsePodmanContainerList returned error: %v", err) + } + if len(containers) != 1 { + t.Fatalf("Expected 1 container, got %d", len(containers)) + } + if containers[0].Name != tt.wantName { + t.Errorf("Expected Name %q, got %q", tt.wantName, containers[0].Name) + } + if containers[0].ID != tt.wantID { + t.Errorf("Expected ID %q, got %q", tt.wantID, containers[0].ID) + } + }) + } +} + func TestCommandExists(t *testing.T) { // Test with a command that should exist on all systems if !commandExists("sh") {