----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63860/#review195130 -----------------------------------------------------------
src/docker/docker.cpp Lines 744-749 (patched) <https://reviews.apache.org/r/63860/#comment274269> Does it make sense to also remove the default here? https://github.com/apache/mesos/blob/51a3bd95bd2d740a39b55634251abeadb561e5c8/include/mesos/mesos.proto#L3120 src/docker/docker.cpp Lines 753 (patched) <https://reviews.apache.org/r/63860/#comment274270> I don't see a list of supported modes on Linux and Windows there. src/docker/docker.cpp Lines 756-779 (patched) <https://reviews.apache.org/r/63860/#comment274272> I prefer to have preprocessor blocks as small as possible, hence I'd suggest the following for your consideration: ``` case ContainerInfo::DockerInfo::HOST: { #ifdef __WINDOWS__ return Error("Unsupported Network mode: " + stringify(network)); #else options.network = "host"; #endif // __WINDOWS__ break; } case ContainerInfo::DockerInfo::BRIDGE: { #ifdef __WINDOWS__ // Windows "nat" network mode is equivalent to Linux "bridge" mode. options.network = "nat"; #else options.network = "bridge"; #endif // __WINDOWS__ break; } case ContainerInfo::DockerInfo::NONE: { options.network = "none"; break; } ``` src/docker/docker.cpp Lines 758-759 (patched) <https://reviews.apache.org/r/63860/#comment274271> Fits one line. - Alexander Rukletsov On Jan. 10, 2018, 1:23 a.m., Akash Gupta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63860/ > ----------------------------------------------------------- > > (Updated Jan. 10, 2018, 1:23 a.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/8/ > > > Testing > ------- > > See https://reviews.apache.org/r/63862/ for test results. > > > Thanks, > > Akash Gupta > >
