mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jojy Varghese" <j...@mesosphere.io>
Subject Re: Review Request 37773: Docker: Adding registry client.
Date Fri, 28 Aug 2015 00:04:32 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message