> 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
> 
>

Reply via email to