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



Looks great! Just some minor nits and a question about authenticating http 
requests even if --authenticate_http=false doesn't require authentication.


src/slave/constants.cpp (line 56)
<https://reviews.apache.org/r/44515/#comment184992>

    "accommodate" (two 'm's)



src/slave/flags.cpp (line 679)
<https://reviews.apache.org/r/44515/#comment184994>

    No need for the final '\n'



src/slave/flags.cpp (line 684)
<https://reviews.apache.org/r/44515/#comment184995>

    I wonder if/when we'll ever deprecate one of these formats.
    https://issues.apache.org/jira/browse/MESOS-2281



src/slave/flags.cpp (lines 685 - 686)
<https://reviews.apache.org/r/44515/#comment184996>

    Can't these lines be joined?



src/slave/flags.cpp (lines 688 - 689)
<https://reviews.apache.org/r/44515/#comment184997>

    Can't these lines be joined?



src/slave/slave.cpp (line 353)
<https://reviews.apache.org/r/44515/#comment184998>

    s/At least...specified/If the http_authenticators flag is not specified, 
the default value will be filled in./



src/slave/slave.cpp (lines 364 - 365)
<https://reviews.apache.org/r/44515/#comment184999>

    How about you just put the whole `"' not "` on the second line with 
`"found..."`



src/slave/slave.cpp (line 366)
<https://reviews.apache.org/r/44515/#comment185000>

    s/''/'/?



src/slave/slave.cpp (line 372)
<https://reviews.apache.org/r/44515/#comment185001>

    So we only load the authenticators and even allow authentication if 
--authenticate_http is set? My understanding is that --authenticate_http=false 
means that both authenticated and unauthenticated requests will be allowed, but 
--authenticate_http=true means that we /require/ all requests to be 
authenticated. That's what the flag help seems to imply.
    Maybe that behavior's only really true for authenticate_frameworks and 
authenticate_slaves



src/tests/mesos.cpp (lines 182 - 184)
<https://reviews.apache.org/r/44515/#comment185005>

    Any reason you can't reuse the previous `path` and `fd` variables?



src/tests/mesos.cpp (line 203)
<https://reviews.apache.org/r/44515/#comment185006>

    s/credentials/http credentials/


- Adam B


On March 9, 2016, 12:47 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44515/
> -----------------------------------------------------------
> 
> (Updated March 9, 2016, 12:47 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Till Toenshoff.
> 
> 
> Bugs: MESOS-4849
>     https://issues.apache.org/jira/browse/MESOS-4849
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added agent flags for HTTP authentication.
> 
> Three command-line flags have been added to the agent to enable HTTP 
> authentication: `--authenticate_http`, `--http_credentials`, and 
> `--http_authenticators`.
> 
> 
> Diffs
> -----
> 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp eb470154f30634b3db439be1c122ff93d3147afe 
>   src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
>   src/tests/mesos.cpp 577072136415191334c74b69a1c0b7eed8e77e8b 
> 
> Diff: https://reviews.apache.org/r/44515/diff/
> 
> 
> Testing
> -------
> 
> `sudo make check` was used to test on both OSX and Ubuntu 14.04.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Reply via email to