mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bernd Mathiske" <be...@mesosphere.io>
Subject Re: Review Request 37999: Implemented http::AuthenticatorManager
Date Wed, 21 Oct 2015 11:46:02 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37999/#review103387
-----------------------------------------------------------



3rdparty/libprocess/src/process.cpp (line 833)
<https://reviews.apache.org/r/37999/#comment161442>

    s/Check the realm/Obtain the realms



3rdparty/libprocess/src/process.cpp (line 843)
<https://reviews.apache.org/r/37999/#comment161437>

    s/ream's/realms'



3rdparty/libprocess/src/process.cpp (line 844)
<https://reviews.apache.org/r/37999/#comment161440>

    Please add an s to this var name. list => plural. This will read better in the subsequent
code. Give it a try!



3rdparty/libprocess/src/process.cpp (line 848)
<https://reviews.apache.org/r/37999/#comment161438>

    Aligning the arguments of the foreach makes it easier to associated them with it instead
of mistaking the second one for something new:
    
    foreach (const std::unique_ptr<Authenticator>& authenticator,
             authenticators_[realm]) {



3rdparty/libprocess/src/process.cpp (line 869)
<https://reviews.apache.org/r/37999/#comment161441>

    Insert blank line.



3rdparty/libprocess/src/process.cpp (line 885)
<https://reviews.apache.org/r/37999/#comment161445>

    Why are we doing this? Why isn't it a failure if there is no success and also no challenge?
Please comment in the source code on this.
    
    My take is that if these authenticators would have liked to pose a challenge, they would
already have done so.



3rdparty/libprocess/src/process.cpp (line 886)
<https://reviews.apache.org/r/37999/#comment161444>

    s/with those/those with



3rdparty/libprocess/src/process.cpp (line 2839)
<https://reviews.apache.org/r/37999/#comment161446>

    Rather than a conditional expression and a complicated comment, let's break this up into
clear if-then-else blocks with dedicated local comments.


- Bernd Mathiske


On Oct. 21, 2015, 3:01 a.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37999/
> -----------------------------------------------------------
> 
> (Updated Oct. 21, 2015, 3:01 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3231
>     https://issues.apache.org/jira/browse/MESOS-3231
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Introduces the authenticator manager, which is a class which handles the actual authentication
procedure during the execution of `ProcessManager::handle()` and it also takes care of the
life cycle of instances of http::Authenticator.
> 
> No tests are added at this point since no public API is generated, the goal of this patch
is to implement the manager and verify nothing breaks afterwards. Authenticator manager tests
proper come in a latter patch.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/Makefile.am fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/authenticator.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/event.hpp 16ddbd77afa6efdf6bad201aa497ee102aa863ae

>   3rdparty/libprocess/include/process/http.hpp 591c1a959057155e1bf0f5bd73352e78d1c15cb3

>   3rdparty/libprocess/src/process.cpp 954d31424bc8f8ecfa78b80513c480f2514ec271 
> 
> Diff: https://reviews.apache.org/r/37999/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


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