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.

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 stdout and stderr in api client #24298

Merged
merged 1 commit into from
Jul 5, 2016

Conversation

allencloud
Copy link
Contributor

@allencloud allencloud commented Jul 4, 2016

This PR did:
Write more specific code when dealing api client stdout and stderr:

  1. fix misuse of dockerCli.Err();
  2. use dockerCli to receive data instead of fmt.Println;

Signed-off-by: allencloud allen.sun@daocloud.io

@vdemeester
Copy link
Member

LGTM 🐸
/cc @dnephin for stack command 👼

@@ -33,7 +33,7 @@ func newConfigCommand(dockerCli *client.DockerCli) *cobra.Command {
}

func runConfig(dockerCli *client.DockerCli, opts configOptions) error {
bundle, err := loadBundlefile(dockerCli.Err(), opts.namespace, opts.bundlefile)
bundle, err := loadBundlefile(dockerCli.Out(), opts.namespace, opts.bundlefile)
Copy link
Member

Choose a reason for hiding this comment

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

I think stderr is correct here. This output is logging, not "normal program output".

Copy link
Member

Choose a reason for hiding this comment

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

For example, if someone is trying to get the output of the bundle into a file, they would direct stdout to a file, but then their will wouldn't be valid because this logging line would be included.

If it goes to stderr the file will be valid JSON.

@dnephin
Copy link
Member

dnephin commented Jul 4, 2016

It looks like api/client/volume/remove.go has the same problem, if you don't mind fixing it at the same time.

@allencloud allencloud force-pushed the fix-stdout-and-stderr-in-api-client branch from 543a734 to 5c46f77 Compare July 4, 2016 15:19
@allencloud
Copy link
Contributor Author

@dnephin Thanks. It is my miss in volume/remove.go. Really sorry. PR updated.
I think I can follow you with your specific explanation. Thanks again. It is reasonable.

@dnephin
Copy link
Member

dnephin commented Jul 4, 2016

LGTM

@allencloud allencloud force-pushed the fix-stdout-and-stderr-in-api-client branch 2 times, most recently from c0ac7e5 to 0859ad4 Compare July 5, 2016 08:53
Signed-off-by: allencloud <allen.sun@daocloud.io>
@allencloud allencloud force-pushed the fix-stdout-and-stderr-in-api-client branch from 0859ad4 to 533bd82 Compare July 5, 2016 15:07
@LK4D4
Copy link
Contributor

LK4D4 commented Jul 5, 2016

LGTM

@LK4D4 LK4D4 merged commit a44e0fe into moby:master Jul 5, 2016
@allencloud allencloud deleted the fix-stdout-and-stderr-in-api-client branch July 5, 2016 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants