----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59101/#review177549 -----------------------------------------------------------
Fix it, then Ship it! src/master/http.cpp Lines 4649-4651 (patched) <https://reviews.apache.org/r/59101/#comment251204> Can do without the lambda here, right? src/master/http.cpp Line 4633 (original), 4653-4654 (patched) <https://reviews.apache.org/r/59101/#comment251205> Indented too far. src/master/http.cpp Lines 4681-4683 (patched) <https://reviews.apache.org/r/59101/#comment251219> Indented too far. src/master/http.cpp Lines 4750-4752 (patched) <https://reviews.apache.org/r/59101/#comment251206> Ditto, can do without this lambda. src/tests/api_tests.cpp Lines 1284 (patched) <https://reviews.apache.org/r/59101/#comment251207> Indented too far. src/tests/api_tests.cpp Lines 1289 (patched) <https://reviews.apache.org/r/59101/#comment251208> Not yours, but: I don't think `this->` is necessary, could you remove it? src/tests/api_tests.cpp Lines 1345 (patched) <https://reviews.apache.org/r/59101/#comment251212> Nit: Let's be consistent with the naming across requests here - you use `v1Response` here, but `v1GetStatusResponse` is used below. I would prefer just `response`, but whatever you decide to use let's make it consistent. src/tests/api_tests.cpp Lines 1346 (patched) <https://reviews.apache.org/r/59101/#comment251210> Indented too far. src/tests/api_tests.cpp Lines 1349 (patched) <https://reviews.apache.org/r/59101/#comment251211> Indented too far. src/tests/api_tests.cpp Line 1323 (original), 1356 (patched) <https://reviews.apache.org/r/59101/#comment251209> Not yours, but could you clean up the indentation here? And below. src/tests/master_maintenance_tests.cpp Lines 82 (patched) <https://reviews.apache.org/r/59101/#comment251217> Looks like this is not necessary. src/tests/master_maintenance_tests.cpp Lines 952 (patched) <https://reviews.apache.org/r/59101/#comment251214> Indented too far. src/tests/master_maintenance_tests.cpp Lines 973 (patched) <https://reviews.apache.org/r/59101/#comment251218> I like the scoping pattern in tests as well, but it looks like the rest of this test doesn't use it currently. I would prefer to remain locally consistent and either just declare `statuses_` here and reassign it later (as is done in the rest of the test), or add local scopes for all of the requests performed in this test. src/tests/master_maintenance_tests.cpp Lines 987 (patched) <https://reviews.apache.org/r/59101/#comment251215> Indented too far. - Greg Mann On June 7, 2017, 1:07 p.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59101/ > ----------------------------------------------------------- > > (Updated June 7, 2017, 1:07 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 `GET_MAINTENANCE_STATUS` v1 > API call, as well as to the `maintenance/status` endpoint, using the > ACL `GetMaintenanceStatus` and the action of the same name as the API > call. > > 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 > src/tests/master_maintenance_tests.cpp > 9edd74a876cd2933d5a5be590a7dd3d10bc54253 > > > Diff: https://reviews.apache.org/r/59101/diff/5/ > > > Testing > ------- > > make check > > > Thanks, > > Alexander Rojas > >
