----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38094/#review109145 -----------------------------------------------------------
3rdparty/libprocess/include/process/authenticator.hpp (line 26) <https://reviews.apache.org/r/38094/#comment168583> Why do you need to include `hashset` here? I don't see any uses of it in the ".hpp" file. 3rdparty/libprocess/include/process/authenticator.hpp (lines 94 - 95) <https://reviews.apache.org/r/38094/#comment168586> Love trailing underscores for class members! 3rdparty/libprocess/src/authenticator.cpp (line 1) <https://reviews.apache.org/r/38094/#comment168584> Do you need a license here? 3rdparty/libprocess/src/authenticator.cpp (lines 25 - 26) <https://reviews.apache.org/r/38094/#comment168593> I think `std::*`s go first. 3rdparty/libprocess/src/authenticator.cpp (line 30) <https://reviews.apache.org/r/38094/#comment168594> You can remove `std::` prefixes. 3rdparty/libprocess/src/authenticator.cpp (lines 39 - 40) <https://reviews.apache.org/r/38094/#comment168595> I see you use the same error message in case something is wrong. Is it done on purpose for security reasons? Or do you think it makes sense to extend the message with specific note in each case? 3rdparty/libprocess/src/authenticator.cpp (lines 67 - 68) <https://reviews.apache.org/r/38094/#comment168596> Let's format this with each condition on a separate line, so it's easier to read. 3rdparty/libprocess/src/tests/http_tests.cpp (line 50) <https://reviews.apache.org/r/38094/#comment168585> Could you please adjust the order? 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1336 - 1337) <https://reviews.apache.org/r/38094/#comment168587> Let's either s/Basic/basic if it's a description, or use the class name (`BasicAuthenticator` with backticks) instead. 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1345 - 1346) <https://reviews.apache.org/r/38094/#comment168588> const? 3rdparty/libprocess/src/tests/http_tests.cpp (line 1352) <https://reviews.apache.org/r/38094/#comment168590> We usually tend to put a verb first, e.g. "Provide no credentials", "Provide wrong credentials". Do you think it makes sense to fix it for consistency? 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1352 - 1360) <https://reviews.apache.org/r/38094/#comment168592> One thing we did in quota tests is we put each scenario in a scope. This way it's more clear to the user where are the boundaries of each scenario, and also it's more clear that a comment prepending the scope is for the whole scope and not for a single following line 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1361 - 1363) <https://reviews.apache.org/r/38094/#comment168591> Do you want to add a test case for wrong principal (maybe even with "testpassword")? - Alexander Rukletsov On Dec. 7, 2015, 2:18 p.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38094/ > ----------------------------------------------------------- > > (Updated Dec. 7, 2015, 2:18 p.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till > Toenshoff. > > > Bugs: MESOS-3232 > https://issues.apache.org/jira/browse/MESOS-3232 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > 3rdparty/libprocess/Makefile.am 6ec6d7989df647f466ac6079738835ffcb2ea8ee > 3rdparty/libprocess/include/process/authenticator.hpp > 5a32e9a38a0bec7aa3faef23b792f3bf3d659d4f > 3rdparty/libprocess/src/CMakeLists.txt > 681f0cfec57e152568da41698c8bdd52c05f65a6 > 3rdparty/libprocess/src/authenticator.cpp PRE-CREATION > 3rdparty/libprocess/src/tests/http_tests.cpp > 2de75ca1c7e224c36b534c368e7379dc158aa5bb > > Diff: https://reviews.apache.org/r/38094/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Alexander Rojas > >
