----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38919/#review102118 -----------------------------------------------------------
Mainly style nits otherwise looks pretty good. src/slave/containerizer/provisioner/docker/spec.hpp (line 38) <https://reviews.apache.org/r/38919/#comment159655> 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);` src/slave/containerizer/provisioner/docker/spec.hpp (line 40) <https://reviews.apache.org/r/38919/#comment159656> I am a bit confused here. Doesn't the `validateManifest` function validates this 4 items ? If so, can we remove this TODO ? src/slave/containerizer/provisioner/docker/spec.cpp (line 38) <https://reviews.apache.org/r/38919/#comment159660> I am assuming .size() returns an `std::size_t` ? Hence just checking for `==0` should suffice ? Ditto for the other 2 cases src/slave/containerizer/provisioner/docker/spec.cpp (line 39) <https://reviews.apache.org/r/38919/#comment159663> We tend to avoid periods at the end of log/error statements. Can you remove them. Ditto for the other 3 occurences src/slave/containerizer/provisioner/docker/spec.cpp (line 50) <https://reviews.apache.org/r/38919/#comment159664> s/va/v1 src/slave/containerizer/provisioner/docker/spec.cpp (line 52) <https://reviews.apache.org/r/38919/#comment159673> s/Number of/Size of src/slave/containerizer/provisioner/docker/spec.cpp (line 59) <https://reviews.apache.org/r/38919/#comment159665> const string& blobSum src/slave/containerizer/provisioner/docker/spec.cpp (line 64) <https://reviews.apache.org/r/38919/#comment159666> Unused variable. Remove it. src/slave/containerizer/provisioner/docker/spec.cpp (line 65) <https://reviews.apache.org/r/38919/#comment159668> Unused variable, remove it. src/slave/containerizer/provisioner/docker/spec.cpp (line 83) <https://reviews.apache.org/r/38919/#comment159669> s/Manisfests/Manifest src/tests/containerizer/provisioner_docker_tests.cpp (line 297) <https://reviews.apache.org/r/38919/#comment159671> 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. src/tests/containerizer/provisioner_docker_tests.cpp (line 386) <https://reviews.apache.org/r/38919/#comment159672> How about: // Test Manifest Validation with empty repeated 'fsLayers' field. Ditto for the following test-case. - Anand Mazumdar On Oct. 6, 2015, 10:35 p.m., Gilbert Song wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38919/ > ----------------------------------------------------------- > > (Updated Oct. 6, 2015, 10:35 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 > >
