> On Dec. 7, 2015, 3:22 p.m., Alexander Rukletsov wrote: > > 3rdparty/libprocess/src/authenticator.cpp, lines 25-26 > > <https://reviews.apache.org/r/38094/diff/15/?file=1128375#file1128375line25> > > > > I think `std::*`s go first.
Not according to all the examples I checked, see `process.cpp` or all the `main.cpp` > On Dec. 7, 2015, 3:22 p.m., Alexander Rukletsov wrote: > > 3rdparty/libprocess/src/authenticator.cpp, lines 39-40 > > <https://reviews.apache.org/r/38094/diff/15/?file=1128375#file1128375line39> > > > > 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? It is actually not an error message but the challenge(-s) to be emited to the client in authentication fails (See the constructor of [Unauthorized](https://github.com/apache/mesos/blob/49296b9a80ec26bf77bc9191fff7b2f5e143b1d2/3rdparty/libprocess/include/process/http.hpp#L521) which takes a vector). Still, the reason why you don't give detailed error messages is because with authentication you want to be quite vague. When you failed to authenticate to a site, it tells you that either your username doesn't exist or your password was wrong, since you rather don't tell which one of the two failed. > On Dec. 7, 2015, 3:22 p.m., Alexander Rukletsov wrote: > > 3rdparty/libprocess/src/tests/http_tests.cpp, line 50 > > <https://reviews.apache.org/r/38094/diff/15/?file=1128376#file1128376line50> > > > > Could you please adjust the order? Not relevant anymore, since the authentication code got its own namespace. > On Dec. 7, 2015, 3:22 p.m., Alexander Rukletsov wrote: > > 3rdparty/libprocess/src/tests/http_tests.cpp, lines 1336-1337 > > <https://reviews.apache.org/r/38094/diff/15/?file=1128376#file1128376line1336> > > > > Let's either s/Basic/basic if it's a description, or use the class name > > (`BasicAuthenticator` with backticks) instead. I would rather not, since you find _Basic_ in pretty much all the literature on the topic (See [RFC 26217](https://tools.ietf.org/html/rfc2617)). > On Dec. 7, 2015, 3:22 p.m., Alexander Rukletsov wrote: > > 3rdparty/libprocess/src/tests/http_tests.cpp, lines 1352-1360 > > <https://reviews.apache.org/r/38094/diff/15/?file=1128376#file1128376line1352> > > > > 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 Great idea. > On Dec. 7, 2015, 3:22 p.m., Alexander Rukletsov wrote: > > 3rdparty/libprocess/src/tests/http_tests.cpp, line 1352 > > <https://reviews.apache.org/r/38094/diff/15/?file=1128376#file1128376line1352> > > > > 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? I didn't even know there was a rule about it (which I find rather extreme). In any case, I think putting a verb first violates the _subject-verb-object_ syntax of english and makes it sound imperative (which it is not). In this case by use of the passive tense I transform the _object_ into the _subject_ and still have a correctly formed english sentence. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38094/#review109145 ----------------------------------------------------------- On Dec. 7, 2015, 3: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, 3: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 > >
