mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 59147: Enabled authorization for v1 calls starting and stopping maintenance.
Date Sat, 10 Jun 2017 18:16:20 GMT

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




src/master/http.cpp
Lines 4377-4379 (patched)
<https://reviews.apache.org/r/59147/#comment251222>

    Can eliminate this lambda and defer to `_startMaintenance` directly, passing `ids.get()`
and `lambda::_1` as parameters.



src/master/http.cpp
Lines 4490-4492 (patched)
<https://reviews.apache.org/r/59147/#comment251227>

    Ditto, can do without this lambda.



src/master/http.cpp
Lines 4560-4562 (patched)
<https://reviews.apache.org/r/59147/#comment251228>

    Can do without this lambda.



src/master/http.cpp
Lines 4673-4675 (patched)
<https://reviews.apache.org/r/59147/#comment251223>

    Ditto, could do without this lambda.



src/tests/api_tests.cpp
Line 1376 (original), 1376 (patched)
<https://reviews.apache.org/r/59147/#comment251230>

    Is there a reason you added authorization to the tests for the '/maintenance/status' endpoint,
but not the other v0 endpoints? Should we update 'src/tests/master_maintenance_tests.cpp'
for all the v0 maintenance endpoints?



src/tests/api_tests.cpp
Lines 1384 (patched)
<https://reviews.apache.org/r/59147/#comment251225>

    Indented too far.



src/tests/api_tests.cpp
Lines 1392 (patched)
<https://reviews.apache.org/r/59147/#comment251226>

    Indented too far.



src/tests/api_tests.cpp
Lines 1397 (patched)
<https://reviews.apache.org/r/59147/#comment251224>

    Could you remove `this->` here as well?



src/tests/api_tests.cpp
Lines 1438-1442 (original), 1470-1481 (patched)
<https://reviews.apache.org/r/59147/#comment251229>

    Could you update this to use the standard `AWAIT_EXPECT_RESPONSE_STATUS_EQ` pattern?
    
    Also, I would prefer if we didn't reasssign the response future; perhaps you could use
a different variable name instead? Here and below.
    
    Let's follow up after this chain lands and do some cleanup of the style in this test file,
I'd be happy to help with that.


- Greg Mann


On June 8, 2017, 1:44 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59147/
> -----------------------------------------------------------
> 
> (Updated June 8, 2017, 1:44 p.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.
> 
> 
> Bugs: MESOS-7415
>     https://issues.apache.org/jira/browse/MESOS-7415
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Enables the use of authorization for the `START_MAINTENANCE` and
> `STOP_MAINTENANCE` v1 API calls, using the ACLs `StartMaintenance` and
> `StopMaintenance` respectively as well the actions of the same name as
> the API calls.
> 
> It also updates the ApiTests to take into account the authorization
> case.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
>   src/master/master.hpp e8ddddf273256b14cde1cac390163f948241757f 
>   src/tests/api_tests.cpp 91b3473452b8e65cab9f2e873837d64a0edf4b54 
> 
> 
> Diff: https://reviews.apache.org/r/59147/diff/5/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


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