----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63860/#review194986 -----------------------------------------------------------
Fix it, then Ship it! src/docker/docker.cpp Lines 742-744 (original), 741-750 (patched) <https://reviews.apache.org/r/63860/#comment274079> Nit/subjective personal opinion: I think that avoiding negative logic in this case would make the block more readable:: ``` if (dockerInfo.has_network()) { network = dockerInfo.network(); } else { // If no network was given, then use the OS specific default. #ifdef __WINDOWS__ network = ContainerInfo::DockerInfo::BRIDGE; #else network = ContainerInfo::DockerInfo::HOST; #endif // __WINDOWS__ } ``` src/docker/docker.cpp Lines 758-759 (patched) <https://reviews.apache.org/r/63860/#comment274078> Fits in one line. - Gaston Kleiman On Jan. 5, 2018, 2:25 p.m., Akash Gupta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63860/ > ----------------------------------------------------------- > > (Updated Jan. 5, 2018, 2:25 p.m.) > > > Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston > Kleiman, Jie Yu, John Kordich, Joseph Wu, and Michael Park. > > > Bugs: MESOS-7342 > https://issues.apache.org/jira/browse/MESOS-7342 > > > Repository: mesos > > > Description > ------- > > The Network enum in DockerInfo is specific to Linux containers. `HOST` > doesn't exist on Windows and `BRIDGE` is `NAT` on Windows. The current > default docker network setting was always `HOST`, which broke the > Windows docker executor. Now, if a specific network isn't given, the > network mode will default to `HOST` on Linux agents and `NAT` on Windows > agents. Also, `BRIDGE` mode will be translated to `NAT` on Windows. > > > Diffs > ----- > > src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed > > > Diff: https://reviews.apache.org/r/63860/diff/7/ > > > Testing > ------- > > See https://reviews.apache.org/r/63862/ for test results. > > > Thanks, > > Akash Gupta > >
