> On Oct. 9, 2015, 3:22 p.m., Anand Mazumdar wrote: > > src/slave/containerizer/provisioner/docker/spec.hpp, line 38 > > <https://reviews.apache.org/r/38919/diff/4/?file=1092075#file1092075line38> > > > > Nit: s/validateManifest/validate > > > > We tend to use non-redundant names. This is clear from the > > signature/comment that this function validates a manifest. > > > > `Option<Error> validate(const docker::DockerImageManifest& manifest);`
I changed it from 'validate' to 'validateManifest', because we will have other validations here, e.g., validateLayout(const std::string), validateImageID(). > On Oct. 9, 2015, 3:22 p.m., Anand Mazumdar wrote: > > src/slave/containerizer/provisioner/docker/spec.hpp, line 40 > > <https://reviews.apache.org/r/38919/diff/4/?file=1092075#file1092075line40> > > > > I am a bit confused here. Doesn't the `validateManifest` function > > validates this 4 items ? > > > > If so, can we remove this TODO ? Thanks for pointing it out. 'Manifest' should be deleted. > On Oct. 9, 2015, 3:22 p.m., Anand Mazumdar wrote: > > src/slave/containerizer/provisioner/docker/spec.cpp, line 38 > > <https://reviews.apache.org/r/38919/diff/4/?file=1092076#file1092076line38> > > > > I am assuming .size() returns an `std::size_t` ? > > > > Hence just checking for `==0` should suffice ? > > > > Ditto for the other 2 cases >From message.ph.h, the returned type is 'int'. > On Oct. 9, 2015, 3:22 p.m., Anand Mazumdar wrote: > > src/slave/containerizer/provisioner/docker/spec.cpp, line 39 > > <https://reviews.apache.org/r/38919/diff/4/?file=1092076#file1092076line39> > > > > We tend to avoid periods at the end of log/error statements. Can you > > remove them. Ditto for the other 3 occurences Thank you for pointing it out. > On Oct. 9, 2015, 3:22 p.m., Anand Mazumdar wrote: > > src/tests/containerizer/provisioner_docker_tests.cpp, line 297 > > <https://reviews.apache.org/r/38919/diff/4/?file=1092077#file1092077line297> > > > > Did you miss adding this block in the last review of this chain ? > > > > Usually we tend to make each review in a chain self-complete by itself > > that can be ideally submitted on it's own. > > > > There might be some exceptions to this in some circumstances though > > when this is not possible. It is originally there, but deleted by mistake during a git rebase. So I add it back. > On Oct. 9, 2015, 3:22 p.m., Anand Mazumdar wrote: > > src/slave/containerizer/provisioner/docker/spec.cpp, line 59 > > <https://reviews.apache.org/r/38919/diff/4/?file=1092076#file1092076line59> > > > > const string& blobSum which will cause an error "error: binding of reference to type 'basic_string<[...]>' to a value of type 'const basic_string<[...]>' drops qualifiers". > On Oct. 9, 2015, 3:22 p.m., Anand Mazumdar wrote: > > src/tests/containerizer/provisioner_docker_tests.cpp, line 386 > > <https://reviews.apache.org/r/38919/diff/4/?file=1092077#file1092077line386> > > > > How about: > > > > // Test Manifest Validation with empty repeated 'fsLayers' field. > > > > Ditto for the following test-case. Nice point. - Gilbert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38919/#review102118 ----------------------------------------------------------- On Oct. 9, 2015, 4:39 p.m., Gilbert Song wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38919/ > ----------------------------------------------------------- > > (Updated Oct. 9, 2015, 4:39 p.m.) > > > Review request for mesos, Jojy Varghese and Timothy Chen. > > > Bugs: MESOS-3099 > https://issues.apache.org/jira/browse/MESOS-3099 > > > Repository: mesos > > > Description > ------- > > Validation of Docker Image Manifests > > > Diffs > ----- > > 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/38919/diff/ > > > Testing > ------- > > make check (Ubuntu14.04 + clang++-3.6) > > > Thanks, > > Gilbert Song > >
