vv9k / podman-api-rs

Rust interface to Podman (libpod).
MIT License
81 stars 11 forks source link

Change return type of `Exec::start` to `Result<tty::Multiplexer<'exec>>` #144

Closed marhkb closed 1 year ago

marhkb commented 1 year ago

In https://github.com/marhkb/pods/pull/451, I implemented a container terminal. For that, I had a look at podman-desktop. They use an exec instance for this

https://github.com/containers/podman-desktop/blob/b12514dc0a894befc530069142c43daf77cb7eef/packages/main/src/plugin/container-registry.ts#L724

In this branch I changed the dignature of Exec::start to return a Multiplexer. I haven't opened a PR for that, because I think I have to check whether tty = true is set in the Opts. But to do that I had to change params to pub(crate). I'm not sure whether there are better approaches.

vv9k commented 1 year ago

I've updated containers-api to expose params at pub(crate) level so that it can be used during Exec creation to set the is_tty field on it that can later be used in the if/else block during Exec::start.

The only problem I see now is that whenever someone does a Execs::get("some-exec-id") there is no way to determine if the exec instance is TTY. I managed to come up with a way to block this Exec instance that is "unchecked" from being used by returning an error in the Exec::start if the is_unchecked field is true but I'm not sure if that's the best approach here. Most of the time the Exec instance will be retrieved from Container::create_exec that has the required knowledge to start the Exec so perhaps removing the whole Execs::get api is the way to go here.

Here is my wip branch if you'd like to take a look: https://github.com/vv9k/podman-api-rs/pull/145

vv9k commented 1 year ago

For now let's leave the API as is and error out when an Exec obtained from Execs::get would be started. I've tested locally by creating two containers with and without TTY and it works in both cases so I'm going to close this issue.