> On Aug. 26, 2015, midnight, Lily Chen wrote:
> >

This was a WIP for you to get the patch :). Anywaz addressed the issues.


> On Aug. 26, 2015, midnight, Lily Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.hpp, line 89
> > <https://reviews.apache.org/r/37773/diff/1/?file=1052557#file1052557line89>
> >
> >     Isn't the authorization server returned as the realm in the 401 
> > response? At least for docker hub?

The tokenmanager design had token managers created per realm. So yes currently 
the design remains same. Can change if required.


> On Aug. 26, 2015, midnight, Lily Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.hpp, line 157
> > <https://reviews.apache.org/r/37773/diff/1/?file=1052557#file1052557line157>
> >
> >     Fix tupo in regiistry

irony :)


> On Aug. 26, 2015, midnight, Lily Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 146
> > <https://reviews.apache.org/r/37773/diff/1/?file=1052558#file1052558line146>
> >
> >     Capitalize Beginning of error messages, same applies throughout.

for internal error messages (that bubbles up), the leaf level messsages are 
recommended to start with lower case. This is so that when the root level 
logger logs the message, it does not look like : "Failed to do operation foo: 
Failed to fetch data".


> On Aug. 26, 2015, midnight, Lily Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 237
> > <https://reviews.apache.org/r/37773/diff/1/?file=1052558#file1052558line237>
> >
> >     Use explicity type instead of auto and remove space between capture and 
> > parameter lists

Saw the pattern in other code. Basically, we are allowed to declare lambdas 
like this. Here the return type is void.


> On Aug. 26, 2015, midnight, Lily Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 360
> > <https://reviews.apache.org/r/37773/diff/1/?file=1052558#file1052558line360>
> >
> >     should we make this some sort of constant?

Didnt seem necessary. Is not used anywhere else. URL class uses literal too.


> On Aug. 26, 2015, midnight, Lily Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 412
> > <https://reviews.apache.org/r/37773/diff/1/?file=1052558#file1052558line412>
> >
> >     Is casting "" to string necessary?

Yes. Compiler cant resolve type 2 times.


> On Aug. 26, 2015, midnight, Lily Chen wrote:
> > src/tests/provisioners/docker_provisioner_tests.cpp, line 95
> > <https://reviews.apache.org/r/37773/diff/1/?file=1052559#file1052559line95>
> >
> >     If we remove a bunch of temporary files during teardown, should we 
> > consider using a TemporaryDirectoryTest?

Not sure if we need a separate class to do it. Moreover, what we are trying to 
accomplish here is to have the blobs saved in a separate dir and not it the 
tests working directory or sandbox.


- Jojy


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37773/#review96446
-----------------------------------------------------------


On Aug. 28, 2015, 12:03 a.m., Jojy Varghese wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> -----------------------------------------------------------
> 
> (Updated Aug. 28, 2015, 12:03 a.m.)
> 
> 
> Review request for mesos, Lily Chen and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/tests/ssl_test.hpp PRE-CREATION 
>   src/Makefile.am 571e1ac0f96b2452797a478680b540f2aab63aab 
>   src/slave/containerizer/provisioners/docker/registry_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/registry_client.cpp 
> PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Reply via email to