----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47794/#review134688 -----------------------------------------------------------
Looking pretty good. Just a couple of comments about wrapping/indentation, and some Options that no longer need to be optional. src/files/files.cpp (lines 152 - 153) <https://reviews.apache.org/r/47794/#comment199548> I'd love a comment explaining what the key/value represent here src/files/files.cpp (line 304) <https://reviews.apache.org/r/47794/#comment199552> s/attached/requestPath/? since it's not necessarily the attached path until it is stripped to something that exists in `authorizations`. In fact, they may have requested a path that doesn't exist Ditto elsewhere src/files/files.cpp (line 307) <https://reviews.apache.org/r/47794/#comment199560> Are we guaranteed that this will always be false on the first iteration? What if the requested path is "/" or "dirname"? Perhaps this should be a `do{}while()`? src/files/files.cpp (lines 309 - 310) <https://reviews.apache.org/r/47794/#comment199559> Can you wrap on `.then(` instead, please? src/files/files.cpp (line 327) <https://reviews.apache.org/r/47794/#comment199555> `path` is guaranteed to be Some by here, so you can remove the `Option` src/files/files.cpp (lines 436 - 439) <https://reviews.apache.org/r/47794/#comment199558> This wrapping/indentation feels off. First of all, we usually put `.then(...` on a new line, indented 2 spaces from the previous. Secondly, if the `bool authorize...{` was on the same line as the previous, then `if (authorized) {` would only be indented two spaces in from `.then(`, so I'd think that should hold, even if we have to wrap the `bool authorized)...{` line. Perhaps: ``` return authorizations[attached](principal) .then([this, offset, length, path, jsonp](bool authorized) -> Future<Response> { if (authorized) { ``` src/tests/files_tests.cpp (line 334) <https://reviews.apache.org/r/47794/#comment199561> s/reads/browse/ src/tests/files_tests.cpp (line 390) <https://reviews.apache.org/r/47794/#comment199562> s/reads/download/ - Adam B On May 24, 2016, 2:39 p.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47794/ > ----------------------------------------------------------- > > (Updated May 24, 2016, 2:39 p.m.) > > > Review request for mesos, Adam B, Benjamin Mahler, Joerg Schad, Michael Park, > and Vinod Kone. > > > Bugs: MESOS-5153 > https://issues.apache.org/jira/browse/MESOS-5153 > > > Repository: mesos > > > Description > ------- > > Adds an optional parameter to the `mesos::internal::Files::attach()` > method. The type of this parameter is a callable object which returns > a future to a boolean and takes as parameter an optional string > representing a principal name. > > The parameter is called, if set, whenever one of the routed endpoints > of the `Files` object is accessed through HTTP. If the callable object > returns a false boolean, then processing of the request is aborted > and a `403 Forbidden` response is returned. > > > Diffs > ----- > > src/files/files.hpp 90acb3406c46c164108deb559af71fb109a5773b > src/files/files.cpp e4b0ada00aabba6553810391f4015a896f8a69a5 > src/tests/files_tests.cpp 5d6620d13babaf0bb7f9c888bb1b4fa2228b6ccd > > Diff: https://reviews.apache.org/r/47794/diff/ > > > Testing > ------- > > On OSX: > `make check` > > > Thanks, > > Alexander Rojas > >
