Conversation
|
Go home @GordonTheTurtle you're drunk. |
|
😂 |
|
seriously amaze On Thu, Apr 9, 2015 at 3:02 PM, Victor Vieux [email protected]
|
|
But where is jenkins? |
|
Amazing but should the test really go? |
|
@icecrime test is broken I'm fixing it just now |
api/server/server.go
Outdated
There was a problem hiding this comment.
why are we ignoring the error?
|
Sorry, I was referring to |
There was a problem hiding this comment.
why are you ignoring the error here?
There was a problem hiding this comment.
lol sorry, I saw this again, and I thought my connection didn't send my earlier comment :)
There was a problem hiding this comment.
There was a problem hiding this comment.
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks for bearing with the visually challenged :)
|
@icecrime mmm I'll reimplement it in integration-cli, is it ok? |
|
yes perfect def not in integration we want to get rid of those :D |
|
Yes I've seen your issue @jfrazelle |
|
Thanks @runcom you are simply awesome! |
|
I wish I could do more :) @tiborvass |
|
@icecrime @tiborvass Added the integration-cli test and rebased |
There was a problem hiding this comment.
#12109 (comment)
also here #11529
I think it make sense and started to use it whenever possible, if it's a bad pattern or unnecessary I'll remove it..
There was a problem hiding this comment.
I'm sure it makes no sense. In that PR I was just tired to understand what's going on.
There was a problem hiding this comment.
Unfortunately I can't see the original context of the outdated diff very easily, so I don't know if it makes sense here. @LK4D4, just for my benefit since I value your opinion, are you saying you consider the "waitgroup to protect goroutine from running out of scope" pattern a bad thing? I'd be interested to know, and be aware of any alternatives :)
There was a problem hiding this comment.
@pwaller the context was the same, just protect a goroutine (in the test) (I suppose)
There was a problem hiding this comment.
seems like errchan <- err is enough without check and then you'll get nil in your e
There was a problem hiding this comment.
Here we lost my comment about not checking for nilness and not closing.
runcom
force-pushed
the
remove-job-wait
branch
2 times, most recently
from
bd815ac to
8febb67
Compare
There was a problem hiding this comment.
Lol, now I wonder why we need runCommand_WithOutput_ here :) maybe to chan we can pass:
fmt.Errorf("%v:\n%s", err, out)
There was a problem hiding this comment.
But then we need != nil check back :P
runcom
force-pushed
the
remove-job-wait
branch
2 times, most recently
from
9a68d98 to
a8df202
Compare
There was a problem hiding this comment.
Haha, sorry :) I'm sure that test in integration was about daemon restart.
There was a problem hiding this comment.
not sure this test belongs here no more..need suggestion
There was a problem hiding this comment.
haha no worries it's not easy. If you need to spawn a new daemon, maybe i'd put it in docker_cli_daemon_test.go and rename it so that it starts with TestDaemon.
There was a problem hiding this comment.
So we're testing that a killed container can have wait called on it after a daemon restart?
There was a problem hiding this comment.
@cpuguy83 maybe I'm missing something here, could you look at the test that I removed in this pr? I wrote in IRC yestarday and I asked and said ok daemon start, cont start, cont kill, daemon restart, cont wait
There was a problem hiding this comment.
It is obviously was test for some bug. I can easy imagine when after restart you have corrupted state and wait will hang forever.
|
I really don't like this test... I know you just moved it from integration to integration-cli. |
|
@runcom Need rebase. Code is ok |
Signed-off-by: Antonio Murdaca <[email protected]>
|
done |
|
LGTM |
1 similar comment
|
LGTM |
|
Nice! |
8 participants
Removed the test also (relying on jobs, sooner or later that tests would be gone I suppose)
Signed-off-by: Antonio Murdaca [email protected]