search

containers / youki

A container runtime written in Rust
https://containers.github.io/youki/
Apache License 2.0
6.2k stars 337 forks source link

Remove `chdir` in container `init` and `start` #2777

Closed Mossaka closed 5 months ago

Mossaka commented 5 months ago

Remove unnecessary chdir in container init and start, the invokes seem not necessary.

Pending comments from https://github.com/containers/youki/issues/2772#issuecomment-2088446225 and will close issue #2772

YJDoc2 commented 5 months ago

Hey, the CI is failing because cross now requires rust 1.77.2 and we use 1.77.1. Have opened https://github.com/containers/youki/pull/2779 which should fix all the CI running issues, so once that is merged, you can rebase on that.

YJDoc2 commented 5 months ago

It is merged, so you can go ahead :+1:

jprendes commented 5 months ago

Hey @Mossaka , I've rebased your branch, I hope that's fine.

Mossaka commented 5 months ago

It seems like the integration test failed. I am not able to tell if it's related to my changes

Mossaka commented 5 months ago

Please wait until we investigat this as it could break pseudo terminal.

Sounds good. That's why I made this PR a draft one until we figure out if it's not breaking existing functionality of youki.

jprendes commented 5 months ago

3 tests failed, all related to setting up tty.

        Error: exec process failed with error error in executing process : failed to setup tty
        : unknown
--- FAIL: TestContainerPTY (0.15s)
        Error: exec process failed with error error in executing process : failed to setup tty
        : unknown
--- FAIL: TestTaskResize (0.08s)
    container_test.go:1890: OCI runtime exec failed: runc did not terminate successfully: exit status 255: exec failed : exec process failed with error error in executing process : failed to setup tty
        : unknown
--- FAIL: TestContainerExecLargeOutputWithTTY (0.13s)
utam0k commented 5 months ago

I was worried about catching it on the test, but it looks like they were testing it! I want to thank our past selves.

utam0k commented 5 months ago

Sorry, but I've created another PR to fix it. https://github.com/containers/youki/pull/2780