Skip to content

Conversation

@dkovba
Copy link
Contributor

@dkovba dkovba commented Nov 4, 2025

Improves the process and network management commands.

@dkovba dkovba requested a review from dcantah November 4, 2025 21:08
@dkovba dkovba force-pushed the dkovba/improvements branch from 9e93b6c to 146e6c5 Compare December 2, 2025 00:42
@dkovba dkovba requested a review from crosbymichael December 2, 2025 01:47
@dkovba dkovba force-pushed the dkovba/improvements branch from 7e66d78 to 8633fe6 Compare December 2, 2025 02:21
switch err.code {
case .invalidArgument:
throw GRPCStatus(code: .invalidArgument, message: "createProcess: failed to create process: \(err)")
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we getting lower fidelity on these error codes? Since we are catching a ContainerizationError it's going to have the original code already as that actually happened. Throwing a new error causes us to lose a bit of information. Is there a reason to do this through this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are catching ContainerizationError to convert it to GRPCStatus. Without this conversion, all errors would become generic GRPC internal errors.

Copy link
Contributor Author

@dkovba dkovba Dec 5, 2025

Choose a reason for hiding this comment

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

It seems reasonable to implement toGRPCStatus in ContainerizationError to reduce the amount of code for error conversion and to convert all possible errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see now. Many of the codes should map well from cz error to grpc status. You could do something like:

extension ContainerizationError {
    /// Convert the ContainerizationError into a GRPCStatus result.
    var status: GRPCStatus {
        let code: GRPCStatus.Code = {
            switch self.code {
            case .notFound: .notFound
            case .exists: .alreadyExists
            case .unsupported: .unimplemented
            case .unknown: .unknown
            case .internalError: .internalError
            case .interrupted: .unavailable
            case .invalidState: .failedPrecondition
            case .invalidArgument: .invalidArgument
            case .timeout: .deadlineExceeded
            default: .internalError
            }
        }()
        return GRPCStatus(
            code: code,
            message: self.description,
            cause: self
        )
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This approach loses the operation context compared to what I've already implemented. However, we can handle more error codes.

@dkovba dkovba requested a review from crosbymichael December 5, 2025 18:43
@dkovba dkovba force-pushed the dkovba/improvements branch from 32d6efb to 22d508b Compare December 5, 2025 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants