Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: don't print to stdout when initializing console fails on Windows #66

Merged
merged 1 commit into from
Mar 13, 2023
Merged

fix: don't print to stdout when initializing console fails on Windows #66

merged 1 commit into from
Mar 13, 2023

Conversation

muesli
Copy link
Contributor

@muesli muesli commented Oct 21, 2022

When stdin/err/out are redirected, console mode can't be changed. That's often expected & acceptable however, so we shouldn't print errors to stdout.

@muesli muesli mentioned this pull request Oct 21, 2022
Closed
@TheReptar TheReptar mentioned this pull request Nov 29, 2022
Merged
@0xdhrv 0xdhrv mentioned this pull request Feb 1, 2023
Open
@WithHolm WithHolm mentioned this pull request Feb 12, 2023
Open
@muesli muesli mentioned this pull request Mar 7, 2023
Closed
@kzys
Copy link
Member

kzys commented Mar 7, 2023

@kevpar @dcantah PTAL

dcantah
dcantah reviewed Mar 7, 2023
View reviewed changes
console_windows.go Outdated Show resolved Hide resolved
dcantah
dcantah approved these changes Mar 9, 2023
View reviewed changes
Copy link
Member

@dcantah dcantah left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'd prefer to get rid of the branches/commented out errors with a little note that we can explore handling redirected io via the conout/in shenanigans, but LGTM!

@muesli
fix: don't print to stdout when initializing console fails on Windows
7b7885a 
When stdin/err/out are redirected, console mode can't be changed.
That's often expected & acceptable however, so we shouldn't print
errors to stdout.

Signed-off-by: Christian Muehlhaeuser <muesli@gmail.com>
@muesli
Copy link
Contributor Author

muesli commented Mar 10, 2023

I'd prefer to get rid of the branches/commented out errors with a little note that we can explore handling redirected io via the conout/in shenanigans, but LGTM!

Done. PTAL!

dcantah
dcantah approved these changes Mar 13, 2023
View reviewed changes
Copy link
Member

@dcantah dcantah left a comment

Choose a reason for hiding this comment

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

LGTM again. @kzys PTAL/merge if you're alright with this, thanks @muesli

@estesp estesp merged commit 1ae8d48 into containerd:main Mar 13, 2023
@muesli muesli deleted the windows-printf branch March 14, 2023 06:34
This was referenced Feb 2, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kzys kzys kzys approved these changes

@dcantah dcantah dcantah approved these changes

@estesp estesp estesp approved these changes

@caarlos0 caarlos0 caarlos0 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Pull Request Review
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@muesli @kzys @caarlos0 @estesp @dcantah