mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rukletsov" <ruklet...@gmail.com>
Subject Re: Review Request 38094: Added implementation of Http Basic authentication scheme.
Date Mon, 07 Dec 2015 14:22:47 GMT

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


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