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 pull image process output from stderr to stdout #3773

Merged
merged 1 commit into from
Dec 20, 2024
Merged

Fix pull image process output from stderr to stdout #3773

merged 1 commit into from
Dec 20, 2024

Conversation

yankay
Copy link
Contributor

@yankay yankay commented Dec 17, 2024

Fix pull image process output by changing the output stream from stderr to stdout

fix: #3395
fix: #3772

AkihiroSuda
AkihiroSuda requested changes Dec 18, 2024
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

This seems to contaminate the stdout of nerdctl run when it incurs pulling an image

@apostasie
Copy link
Contributor

This seems to contaminate the stdout of nerdctl run when it incurs pulling an image

Just tested.
It seems docker run does output pull information on stderr, unlike docker pull

@apostasie
Copy link
Contributor

@yankay maybe we should have tests for that stuff?

@yankay
Copy link
Contributor Author

yankay commented Dec 19, 2024

Let me investigate :-)

@yankay yankay force-pushed the fix-pull-image-process-to-stderr2 branch 9 times, most recently from 27ffb55 to 049eed8 Compare December 19, 2024 10:02
@yankay
Copy link
Contributor Author

yankay commented Dec 19, 2024 •
edited
Loading

HI @apostasie @AkihiroSuda
Could you please review the PR again?

apostasie
apostasie reviewed Dec 19, 2024
View reviewed changes
cmd/nerdctl/image/image_pull_linux_test.go Outdated
},
{
Description: "Run Container with image pull - output should be in stderr",
Setup: func(data test.Data, helpers test.Helpers) {
Copy link
Contributor

Choose a reason for hiding this comment

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

That should be Cleanup instead of Setup (note: Cleanup is called both before Setup and after the test completion)

apostasie
apostasie reviewed Dec 19, 2024
View reviewed changes
Copy link
Contributor

@apostasie apostasie left a comment •
edited by yankay
Loading

Choose a reason for hiding this comment

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

Just a couple of nits on the test.

@yankay yankay force-pushed the fix-pull-image-process-to-stderr2 branch 2 times, most recently from 047dfc3 to c5116fa Compare December 20, 2024 02:06
@yankay
fix pull image process output from stderr to stdout
3dbd24c 
Signed-off-by: Kay Yan <kay.yan@daocloud.io>
@yankay yankay force-pushed the fix-pull-image-process-to-stderr2 branch from c5116fa to 3dbd24c Compare December 20, 2024 02:07
@yankay
Copy link
Contributor Author

yankay commented Dec 20, 2024

Just a couple of nits on the test.

Thanks @apostasie
It has changed

@AkihiroSuda AkihiroSuda added this to the v2.0.3 milestone Dec 20, 2024
AkihiroSuda
AkihiroSuda approved these changes Dec 20, 2024
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit c5b57fa into containerd:main Dec 20, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apostasie apostasie apostasie approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.0.3
3 participants
@yankay @apostasie @AkihiroSuda