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

Add String method so quieted output displays properly #1090

Merged
merged 1 commit into from
May 29, 2018

Conversation

zkry
Copy link
Contributor

@zkry zkry commented May 29, 2018

Fixes #1089 as well as addresses moby/moby#36812

- What I did
It turns out that the when the --stream flag is provided, a struct with a few different fields is used and when trying to print it in the quiet case, it uses a default string representation of the struct. I added a custom String method to only display the relevant data.

- Description for the changelog
Fixed bug displaying garbage output for build command when --stream and --quiet flags combined

Fixes docker#1089

Signed-off-by: Zachary Romero <zacromero3@gmail.com>
@vdemeester
Copy link
Collaborator

We should wait for https://jenkins.dockerproject.org/job/docker/job/cli/job/PR-1090 to be done (but somehow there is no node 😭)

Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

SGTM 🐸

@thaJeztah
Copy link
Member

Before this patch:

cat <<EOF> Dockerfile
FROM busybox
RUN echo foo
EOF

docker build --quiet --stream .
&{%!!(MISSING)s(chan error=0xc42001a960) sha256:a0656916e1769300a230126221760c5d997870d249445f34e7884017a2bf1454
 %!!(MISSING)s(*bytes.Buffer=&{[] 0 0 [0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0]}) %!!(MISSING)s(chan struct {}=0xc42001a9c0) {%!!(MISSING)s(int32=0) %!!(MISSING)s(uint32=0)}}

With this patch:

cat <<EOF> Dockerfile
FROM busybox
RUN echo foo
EOF

docker build --quiet --stream .
sha256:a0656916e1769300a230126221760c5d997870d249445f34e7884017a2bf1454

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@@ -122,6 +122,10 @@ func (bw *bufferedWriter) flushBuffer() {
bw.mu.Unlock()
}

func (bw *bufferedWriter) String() string {
return fmt.Sprintf("%s", bw.Writer)
Copy link
Member

@thaJeztah thaJeztah May 29, 2018

Choose a reason for hiding this comment

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

Actually, wondering if there's a better solution to convert it to a string 🤔 (at least could be simplified to fmt.Sprint(bw.Writer))

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

Yeah, let's leave it as-is for now

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.

5 participants