Skip to content
Open
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
21 changes: 21 additions & 0 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,27 @@ let package = Package(
name: "SocketForwarderTests",
dependencies: ["SocketForwarder"]
),
.testTarget(
name: "ContainerCommandsTests",
dependencies: [
"ContainerCommands",
"ContainerClient",
.product(name: "Containerization", package: "containerization"),
]
),
.testTarget(
name: "CLITests",
dependencies: [
.product(name: "AsyncHTTPClient", package: "async-http-client"),
.product(name: "Containerization", package: "containerization"),
.product(name: "ContainerizationExtras", package: "containerization"),
.product(name: "ContainerizationOS", package: "containerization"),
"ContainerBuild",
"ContainerClient",
"ContainerNetworkService",
],
path: "Tests/CLITests"
),
.target(
name: "ContainerVersion",
dependencies: [
Expand Down
17 changes: 7 additions & 10 deletions Sources/ContainerCommands/Container/ContainerKill.swift
Original file line number Diff line number Diff line change
Expand Up @@ -50,21 +50,18 @@ extension Application {
}

public mutating func run() async throws {
let set = Set<String>(containerIds)
let allContainers = try await ClientContainer.list()

var containers = try await ClientContainer.list().filter { c in
c.status == .running
}
if !self.all {
containers = containers.filter { c in
set.contains(c.id)
}
}
let containersToSignal = try self.all
? allContainers.filter { $0.status == .running }
: ContainerStop.containers(matching: containerIds, in: allContainers)

let runningContainers = containersToSignal.filter { $0.status == .running }

let signalNumber = try Signals.parseSignal(signal)

var failed: [String] = []
for container in containers {
for container in runningContainers {
do {
try await container.kill(signalNumber)
print(container.id)
Expand Down
38 changes: 27 additions & 11 deletions Sources/ContainerCommands/Container/ContainerStop.swift
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ import ContainerizationError
import ContainerizationOS
import Foundation

package protocol ContainerIdentifiable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactoring this resolution logic into a shared helper is a great move. It keeps Stop and Kill consistent and makes testing way easier.

var id: String { get }
var status: RuntimeStatus { get }
}

extension ClientContainer: ContainerIdentifiable {}

extension Application {
public struct ContainerStop: AsyncParsableCommand {
public init() {}
Expand All @@ -45,25 +52,20 @@ extension Application {
var containerIds: [String] = []

public func validate() throws {
if containerIds.count == 0 && !all {
if containerIds.isEmpty && !all {
throw ContainerizationError(.invalidArgument, message: "no containers specified and --all not supplied")
}
if containerIds.count > 0 && all {
if !containerIds.isEmpty && all {
throw ContainerizationError(
.invalidArgument, message: "explicitly supplied container IDs conflict with the --all flag")
}
}

public mutating func run() async throws {
let set = Set<String>(containerIds)
var containers = [ClientContainer]()
if self.all {
containers = try await ClientContainer.list()
} else {
containers = try await ClientContainer.list().filter { c in
set.contains(c.id)
}
}
let allContainers = try await ClientContainer.list()
let containers = try self.all
? allContainers
: Self.containers(matching: containerIds, in: allContainers)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should handle ambiguous prefix matches here.

Currently, allContainers.first(where:) will silently pick the first matching container. If I have containers abc and abd, running stop ab might stop abc just because it appears first in the list.

It would be safer to filter all matches and throw an error if matches.count > 1 (unless there's an exact match). This aligns with how Docker handles ambiguous prefixes.


let opts = ContainerStopOptions(
timeoutInSeconds: self.time,
Expand Down Expand Up @@ -104,5 +106,19 @@ extension Application {

return failed
}

static func containers<C: ContainerIdentifiable>(
matching containerIds: [String],
in allContainers: [C]
) throws -> [C] {
var matched: [C] = []
for containerId in containerIds {
guard let container = allContainers.first(where: { $0.id == containerId || $0.id.starts(with: containerId) }) else {
throw ContainerizationError(.notFound, message: "no such container: \(containerId)")
}
matched.append(container)
}
return matched
}
}
}
61 changes: 61 additions & 0 deletions Tests/ContainerCommandsTests/ContainerStopValidationTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
//===----------------------------------------------------------------------===//
// Copyright © 2025 Apple Inc. and the container project authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//===----------------------------------------------------------------------===//

import ContainerClient
import ContainerCommands
import ContainerizationError
import Testing

@testable import ContainerCommands

struct ContainerStopValidationTests {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a test case for ambiguous prefixes?

We should verify that stop ab throws an error if both abc and abd exist. It's an important edge case to prevent accidental stops.

struct StubContainer: ContainerIdentifiable {
let id: String
let status: RuntimeStatus
}

@Test
func resolvesExactIds() throws {
let containers = [StubContainer(id: "abc123", status: .running)]
let matched = try Application.ContainerStop.containers(matching: ["abc123"], in: containers)
#expect(matched.count == 1)
#expect(matched.first?.id == "abc123")
}

@Test
func resolvesIdPrefixes() throws {
let containers = [
StubContainer(id: "abcdef", status: .running),
StubContainer(id: "123456", status: .running),
]
let matched = try Application.ContainerStop.containers(matching: ["abc"], in: containers)
#expect(matched.count == 1)
#expect(matched.first?.id == "abcdef")
}

@Test
func throwsForMissingContainers() throws {
let containers = [StubContainer(id: "abcdef", status: .running)]
#expect {
_ = try Application.ContainerStop.containers(matching: ["missing"], in: containers)
} throws: { error in
guard let error = error as? ContainerizationError else {
return false
}
return error.code == .notFound
}
}
}