> On Oct. 9, 2015, 1:12 p.m., Anand Mazumdar wrote: > > src/slave/containerizer/provisioner/docker/spec.hpp, line 18 > > <https://reviews.apache.org/r/38901/diff/6/?file=1092471#file1092471line18> > > > > We generally prefer header files that are `complete` i.e. they compile > > on their own. Can you ensure that we include `stout/json.hpp` here since it > > has `JSON::Object`. Also , do a sweep of the other includes.
Thank you for reminding. For your recommendation of sweeping, the others are included and used in the next patch, which is depending this one. So I leave them here. > On Oct. 9, 2015, 1:12 p.m., Anand Mazumdar wrote: > > src/slave/containerizer/provisioner/docker/spec.hpp, line 22 > > <https://reviews.apache.org/r/38901/diff/6/?file=1092471#file1092471line22> > > > > Do we need this ? If not, remove this include. Ditto. It is used in the next patch. > On Oct. 9, 2015, 1:12 p.m., Anand Mazumdar wrote: > > src/slave/containerizer/provisioner/docker/spec.cpp, line 20 > > <https://reviews.apache.org/r/38901/diff/6/?file=1092472#file1092472line20> > > > > Do you need this ? If not remove it Ditto. (referring to https://reviews.apache.org/r/38919/diff/4#index_header) > On Oct. 9, 2015, 1:12 p.m., Anand Mazumdar wrote: > > src/slave/containerizer/provisioner/docker/spec.cpp, line 24 > > <https://reviews.apache.org/r/38901/diff/6/?file=1092472#file1092472line24> > > > > Do you need this ? If not remove it ditto. > On Oct. 9, 2015, 1:12 p.m., Anand Mazumdar wrote: > > src/tests/containerizer/provisioner_docker_tests.cpp, line 331 > > <https://reviews.apache.org/r/38901/diff/6/?file=1092473#file1092473line331> > > > > Nit: Does it make sense to change the ASSERT_EQ to EXPECT_EQ ? > > > > Might help the user to see a list of all the things that failed instead > > of just aborting at the first failure. This is a good point. - Gilbert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38901/#review102105 ----------------------------------------------------------- On Oct. 9, 2015, 2:39 p.m., Gilbert Song wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38901/ > ----------------------------------------------------------- > > (Updated Oct. 9, 2015, 2:39 p.m.) > > > Review request for mesos, Jojy Varghese and Timothy Chen. > > > Bugs: MESOS-2972 > https://issues.apache.org/jira/browse/MESOS-2972 > > > Repository: mesos > > > Description > ------- > > Serialize Docker Image Spec as Protobuf > > > Diffs > ----- > > src/CMakeLists.txt 828dbb690841c561816811dfbb044aa3afead89d > src/Makefile.am d855cb83277c3e0e2ee3feacaf6ad0962223ef6e > src/slave/containerizer/provisioner/docker/message.proto > bbac2e6c1f40a7ca3f9227baca56a44cd43f58c6 > src/slave/containerizer/provisioner/docker/spec.hpp PRE-CREATION > src/slave/containerizer/provisioner/docker/spec.cpp PRE-CREATION > src/tests/containerizer/provisioner_docker_tests.cpp > d895eb9d0723e52cff8b21ef2deeaef1911d019c > > Diff: https://reviews.apache.org/r/38901/diff/ > > > Testing > ------- > > make check (ubuntu 14.04 + clang++-3.6) > > > Thanks, > > Gilbert Song > >
