-
Notifications
You must be signed in to change notification settings - Fork 606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix panic 'provided file is not a console' #3434
Conversation
Ping @AkihiroSuda - pending green CI. |
Ahhhh windows, windows. @sondavidb @jsturtevant any idea for me on this? |
1ca6160
to
4442b17
Compare
Was trying to set up containerd on my local machine and it has been a lot more difficult than I anticipated 😓 Looks like the change you made had the integration pass but not the canary. Is this test known to be flaky? Looking through the changes it seems fine so I'm pretty stumped as well |
@sondavidb I think Might be a recent regression, and unrelated to this changeset, as my other PRs get the failure too. Investigating... |
Test appears broken on main. Moving discussion to #3437 |
@AkihiroSuda besides what appears to be an unrelated regression / flakyness (#3437) this is ready. |
As outlined in containerd#3433, containerd/console will panic on a call to console.Current(). This patch provides a simple consoleutil wrapper that will return an error instead. Note that part of containerd#3300 is being reverted, as no longer necessary. This patch does not try to be "smart" and does not check the status of stdin/out/err otherwise or if it is consistent with user provided flags, but merely ensures we do not crash. Signed-off-by: apostasie <[email protected]>
4442b17
to
06a257e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
See commit message.
Fixes #3433
This should put this panic issue to rest.