Remove jobs from daemon/networkdriver/bridge#12092
Conversation
|
@tiborvass Something failed. |
tiborvass
force-pushed
the
no-jobs-networkdriver-bridge
branch
5 times, most recently
from
e524e35 to
90d2124
Compare
daemon/container.go
Outdated
There was a problem hiding this comment.
I don't think you need _ = or the commented code here.
There was a problem hiding this comment.
it was to make it obvious the error was ignored. I can remove it if you don't want it.
There was a problem hiding this comment.
i kinda like the _, but dont care either way
There was a problem hiding this comment.
Why we ignoring it? Maybe at least log it? Or log inside and remove returning error.
There was a problem hiding this comment.
@tiborvass What's here? This error is never checked.
There was a problem hiding this comment.
@LK4D4 I know, that's why I put the _ =. I'm fine with logging it. Info?
There was a problem hiding this comment.
I mean that this error not checked at all in code. No one uses it. Maybe better to log it inside function and not return err.
I thought about Warn
tiborvass
force-pushed
the
no-jobs-networkdriver-bridge
branch
from
90d2124 to
14f6882
Compare
There was a problem hiding this comment.
I also hope this is alright
There was a problem hiding this comment.
^^ Just to be sure, this was alright? (In case someone visits this at a later stage)
There was a problem hiding this comment.
@thaJeztah Well I am not sure, I needed it to fix an error case and it makes sense to me, but maybe I could be missing something. That's why I need people to review it. @LK4D4 maybe you know why this would be bad?
There was a problem hiding this comment.
0 port is random port. What error you tried to fix?
tiborvass
force-pushed
the
no-jobs-networkdriver-bridge
branch
2 times, most recently
from
d9250ec to
44a762e
Compare
|
Ping @jfrazelle @crosbymichael |
|
LGTM |
There was a problem hiding this comment.
Should be %T :) Yes, I'm checking with vet all PRs now until we merge autocheck.
|
@tiborvass Is it in plans to convert |
|
@LK4D4 I don't know if it's in the plans, but I did it. It's |
tiborvass
force-pushed
the
no-jobs-networkdriver-bridge
branch
from
44a762e to
30325ac
Compare
|
@tiborvass No, it is still |
|
@LK4D4 I see. I'll try to do that. |
|
@tiborvass In this PR or later? |
|
@LK4D4 i can do it here if you want |
|
@tiborvass Would be cool. |
|
@LK4D4 Actually let's leave it for another PR. This PR is about networkdriver/bridge, you're asking to put networkdriver as a NetworkService in Daemon. |
|
ok |
Signed-off-by: Tibor Vass <[email protected]>
tiborvass
force-pushed
the
no-jobs-networkdriver-bridge
branch
from
30325ac to
5358232
Compare
|
Ping @LK4D4 :) |
There was a problem hiding this comment.
@LK4D4 this here would panic without my change in nat/nat.go. It is replacing GetenvInt("ContainerPort"), which was returning 0 in case ParseInt was erroring out: https://github.com/docker/docker/blob/2052d01b1e52a4b1fa79fdc73c52a7b52f6d88a7/engine/env.go#L90
There was a problem hiding this comment.
Don't know. Maybe we can leave it as it is. But this port type is really awful.
There was a problem hiding this comment.
@LK4D4 at least I don't think I'm modifying any behavior in this PR. Actually the only modification I'm making is that if somebody types in a negative port, then this PR will error out, while master would just consider it 0 (to be verified but that's what i see from the code)
|
LGTM |
Remove jobs from daemon/networkdriver/bridge
7 participants
Signed-off-by: Tibor Vass [email protected]
Ping @LK4D4