search

docker / app

Make your Docker Compose applications reusable, and share them on Docker Hub
Apache License 2.0
1.57k stars 176 forks source link

[rfd] Use dockerCli.Out() / Err() #664

Closed thaJeztah closed 4 years ago

thaJeztah commented 4 years ago

Quick 5-minute write-up;

This should likely not make a difference for most situations, but try to keep a single source of truth where to print output

There's still some occurences of os.Stdout to detect if a terminal is attached, so perhaps we need a central solution for that

silvin-lubecki commented 4 years ago

😅 

=== FAIL: e2e TestImageList (90.84s)
panic: runtime error: index out of range [recovered]
    panic: runtime error: index out of range

goroutine 194 [running]:
testing.tRunner.func1(0xc0000a3700)
    /usr/local/go/src/testing/testing.go:830 +0x392
panic(0xa16300, 0xfdbe10)
    /usr/local/go/src/runtime/panic.go:522 +0x1b5
github.com/docker/app/e2e.insertBundles(0xc0000a3700, 0xc0003ec2a0, 0x6, 0x6, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
    /go/src/github.com/docker/app/e2e/images_test.go:26 +0xfc5
github.com/docker/app/e2e.TestImageList.func1(0xc0002fcb00, 0xf, 0xc0002d8e20, 0xe, 0xc00020c000, 0x4, 0x4, 0x0, 0x0, 0x0, ...)
    /go/src/github.com/docker/app/e2e/images_test.go:52 +0x149
github.com/docker/app/e2e.runWithDindSwarmAndRegistry(0xc0000a3700, 0xc000493f88)
    /go/src/github.com/docker/app/e2e/helper_test.go:86 +0x1791
github.com/docker/app/e2e.TestImageList(0xc0000a3700)
    /go/src/github.com/docker/app/e2e/images_test.go:47 +0x4e
testing.tRunner(0xc0000a3700, 0xae09a0)
    /usr/local/go/src/testing/testing.go:865 +0xc0
created by testing.(*T).Run
    /usr/local/go/src/testing/testing.go:916 +0x35a
FAIL    github.com/docker/app/e2e   90.843s
thaJeztah commented 4 years ago

LOL, yes, I saw the failures. That's what you get with a 5-minute PoC 😂

I guess it's because the e2e tests explicitly silence the CLI (but not yet familiar with the tests here; was wondering if the same approach was taken on the docker/cli repo)

ndeloof commented 4 years ago

I can run e2e tests without getting this error, wonder there's something CI specific to trigger this?

thaJeztah commented 4 years ago

I can run e2e tests without getting this error, wonder there's something CI specific to trigger this?

Interesting; could it be if there's no TTY attached or something? (I haven't looked into it yet)

chris-crone commented 4 years ago

I think we've actually implemented this in the time since you opened this PR @thaJeztah :) Thanks for the suggestion!

I'll close this and we'll open other issues if we find places we forgot to do this.