> On April 27, 2016, 12:59 p.m., Adam B wrote: > > src/tests/slave_authorization_tests.cpp, line 179 > > <https://reviews.apache.org/r/46319/diff/8/?file=1363034#file1363034line179> > > > > { belongs on newline.
Ups. On Alex's request I now removed any additional state from the fixture and inline agent setup into each test. This leads to an empty class body which again fits on a single line. > On April 27, 2016, 12:59 p.m., Adam B wrote: > > src/tests/slave_authorization_tests.cpp, line 187 > > <https://reviews.apache.org/r/46319/diff/8/?file=1363034#file1363034line187> > > > > "Prefer trailing underscores for use as member fields", so you've got > > this reversed. > > Alexander Rukletsov wrote: > Unfortunately, that rule is not really welcomed by all members of the > community, maybe we should even remove it (though I personally love it). As I now removed this code this does comment does not apply anymore. > On April 27, 2016, 12:59 p.m., Adam B wrote: > > src/slave/http.cpp, line 629 > > <https://reviews.apache.org/r/46319/diff/8/?file=1363032#file1363032line629> > > > > Is there any standard for what order these [pid, limiter, request] > > should go in? I cannot find any explicit guidance on this in the style guide. My intuition here was to mention `Http` member-like variables (`pid` and `limiter`) before parameter-like variables (`request`). > On April 27, 2016, 12:59 p.m., Adam B wrote: > > src/tests/slave_authorization_tests.cpp, lines 204-207 > > <https://reviews.apache.org/r/46319/diff/8/?file=1363034#file1363034line204> > > > > I thought this was done in SetUp() now Ups. I removed the fixture's `SetUp` so this comment does not apply anymore. > On April 27, 2016, 12:59 p.m., Adam B wrote: > > src/tests/slave_authorization_tests.cpp, lines 252-254 > > <https://reviews.apache.org/r/46319/diff/8/?file=1363034#file1363034line252> > > > > How is this testing with "no authorizer"? Doesn't SetUp use > > MockAuthorizer? Looks like MockAuthorizer::authorized() returns true by > > default. Might as well just remove this test. Thanks for catching this. The original intention here was to create an agent w/o authorization, but at some point I regressed to passing in an authorizer. I now fixed the way the agent is created, and propose that we leave this test in. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46319/#review130756 ----------------------------------------------------------- On April 27, 2016, 2:18 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46319/ > ----------------------------------------------------------- > > (Updated April 27, 2016, 2:18 p.m.) > > > Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht. > > > Bugs: MESOS-5164 > https://issues.apache.org/jira/browse/MESOS-5164 > > > Repository: mesos > > > Description > ------- > > Added authorization to agents' `/monitor/statistics` endpoints. > > > Diffs > ----- > > src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e > src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 > src/tests/slave_authorization_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/46319/diff/ > > > Testing > ------- > > make check (OS X, clang w/o optimization) > > > Thanks, > > Benjamin Bannier > >
