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:03:24 GMT

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



3rdparty/libprocess/include/process/authenticator.hpp (line 52)
<https://reviews.apache.org/r/37999/#comment161405>

    Duplicate text.



3rdparty/libprocess/include/process/authenticator.hpp (line 53)
<https://reviews.apache.org/r/37999/#comment161409>

    Suggested rephrasing: "These parameters come in the form of a map<string, string>.
Their interpretation depends on the specific authenticator instance that receives them. For
example, a basic authenticator could expect mappings from user names to encrypted passwords."



3rdparty/libprocess/include/process/authenticator.hpp (line 69)
<https://reviews.apache.org/r/37999/#comment161411>

    s/It/Its
    
    better, simpler, replace these two sentences with:
    "Possible outcomes:"



3rdparty/libprocess/include/process/authenticator.hpp (line 74)
<https://reviews.apache.org/r/37999/#comment161412>

    s/possible/



3rdparty/libprocess/include/process/authenticator.hpp (line 77)
<https://reviews.apache.org/r/37999/#comment161413>

    s/finish/finished



3rdparty/libprocess/include/process/authenticator.hpp (line 79)
<https://reviews.apache.org/r/37999/#comment161414>

    s/instance of an



3rdparty/libprocess/include/process/authenticator.hpp (line 80)
<https://reviews.apache.org/r/37999/#comment161415>

    s/The contents must be ready to be used/This will be used



3rdparty/libprocess/include/process/authenticator.hpp (line 86)
<https://reviews.apache.org/r/37999/#comment161416>

    used? in what way? by whom?
    
    Suggestion: "A standard HTTP authenticator will typically only read the 'WWW-Authenticate'
header of the request."



3rdparty/libprocess/include/process/authenticator.hpp (line 98)
<https://reviews.apache.org/r/37999/#comment161417>

    s/use/be used
    s/a HTTP/an HTTP



3rdparty/libprocess/include/process/authenticator.hpp (line 107)
<https://reviews.apache.org/r/37999/#comment161419>

    s/are/is (contents is singular)
    
    s/a HTTP/an HTTP 
    
    (Please check all other places. If "a" is followed by "H" in an abbreviation, which is
therefore pronounced "aytsh", this constitutes an "a" followed by a vowel sound, so it becomes
"an)



3rdparty/libprocess/include/process/http.hpp (line 494)
<https://reviews.apache.org/r/37999/#comment161421>

    s/for/with/



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

    It may not be 100% clear to everybody what the parameters mean and what he method is supposed
to accomplish. Probably better to explain here with a quick comment. (Instead of letting the
implementation below become the specification of what might be intended.)
    
    (No need to repeat such comments in the AuthenticatorProcess declaration.)



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

    It is not immediately clear what this parameter does. Please write a comment for this
method.



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

    Please move this to line 667.



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

    Please also document what the Future return type does.



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

    s/provides a has/provide a hash



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

    Insert a blank line, please!



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

    Suggestion: break this up into two calls.
    
    removeAllAuthenticators()
    removeAuthenticator(<param>)


- 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