Re: Review Request 59100: Enabled authorization for v1 API call GET_MAINTENANCE_SCHEDULE.

2017-06-12 Thread Alexander Rojas 


> On June 10, 2017, 7:38 p.m., Greg Mann wrote:
> > src/master/http.cpp
> > Line 4030 (original), 4030-4034 (patched)
> > 
> >
> > Could you run 'support/generate-endpoint-help.py' and submit a RR for 
> > updates to the HTTP endpoint docs?

will do in a follow up patch.


- Alexander


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


On June 7, 2017, 3:02 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59100/
> ---
> 
> (Updated June 7, 2017, 3:02 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_SCHEDULE`
> v1 API call, using the ACL `GetMaintenanceSchedule` 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 e8f273256b14cde1cac390163f948241757f 
>   src/tests/api_tests.cpp 91b3473452b8e65cab9f2e873837d64a0edf4b54 
> 
> 
> Diff: https://reviews.apache.org/r/59100/diff/6/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 59100: Enabled authorization for v1 API call GET_MAINTENANCE_SCHEDULE.

2017-06-10 Thread Greg Mann 

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




src/master/http.cpp
Line 4030 (original), 4030-4034 (patched)


Could you run 'support/generate-endpoint-help.py' and submit a RR for 
updates to the HTTP endpoint docs?


- Greg Mann


On June 7, 2017, 1:02 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59100/
> ---
> 
> (Updated June 7, 2017, 1:02 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_SCHEDULE`
> v1 API call, using the ACL `GetMaintenanceSchedule` 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 e8f273256b14cde1cac390163f948241757f 
>   src/tests/api_tests.cpp 91b3473452b8e65cab9f2e873837d64a0edf4b54 
> 
> 
> Diff: https://reviews.apache.org/r/59100/diff/6/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 59100: Enabled authorization for v1 API call GET_MAINTENANCE_SCHEDULE.

2017-06-10 Thread Greg Mann 

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



Could you update the description to reflect that this also enables 
authorization on the v0 `/master/maintenance/schedule` endpoint? On other 
reviews as necessary too.

- Greg Mann


On June 7, 2017, 1:02 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59100/
> ---
> 
> (Updated June 7, 2017, 1:02 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_SCHEDULE`
> v1 API call, using the ACL `GetMaintenanceSchedule` 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 e8f273256b14cde1cac390163f948241757f 
>   src/tests/api_tests.cpp 91b3473452b8e65cab9f2e873837d64a0edf4b54 
> 
> 
> Diff: https://reviews.apache.org/r/59100/diff/6/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 59100: Enabled authorization for v1 API call GET_MAINTENANCE_SCHEDULE.

2017-06-10 Thread Greg Mann 

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




src/master/http.cpp
Lines 4073 (patched)


Indented too far.



src/master/http.cpp
Lines 4116 (patched)


How about logging a warning when `approved.isError()`?



src/master/http.cpp
Lines 4290 (patched)


Indented too far.



src/tests/api_tests.cpp
Lines 1173 (patched)


Indented too far.



src/tests/api_tests.cpp
Lines 1246 (patched)


Indented too far.



src/tests/api_tests.cpp
Lines 1250 (patched)


Indented too far.



src/tests/api_tests.cpp
Line 1225 (original), 1256 (patched)


Indented too far.


- Greg Mann


On June 7, 2017, 1:02 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59100/
> ---
> 
> (Updated June 7, 2017, 1:02 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_SCHEDULE`
> v1 API call, using the ACL `GetMaintenanceSchedule` 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 e8f273256b14cde1cac390163f948241757f 
>   src/tests/api_tests.cpp 91b3473452b8e65cab9f2e873837d64a0edf4b54 
> 
> 
> Diff: https://reviews.apache.org/r/59100/diff/6/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 59100: Enabled authorization for v1 API call GET_MAINTENANCE_SCHEDULE.

2017-06-07 Thread Alexander Rojas 

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

(Updated June 7, 2017, 3:02 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_SCHEDULE`
v1 API call, using the ACL `GetMaintenanceSchedule` and the action
of the same name as the API call.

It also updates the ApiTests to take into account the authorization
case.


Diffs (updated)
-

  src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
  src/master/master.hpp e8f273256b14cde1cac390163f948241757f 
  src/tests/api_tests.cpp 91b3473452b8e65cab9f2e873837d64a0edf4b54 


Diff: https://reviews.apache.org/r/59100/diff/6/

Changes: https://reviews.apache.org/r/59100/diff/5-6/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 59100: Enabled authorization for v1 API call GET_MAINTENANCE_SCHEDULE.

2017-06-02 Thread Greg Mann 

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




src/tests/api_tests.cpp
Lines 1249 (patched)


What do you think about using just `schedule` instead of `respSchedule` 
here?


- Greg Mann


On June 2, 2017, 12:39 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59100/
> ---
> 
> (Updated June 2, 2017, 12:39 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_SCHEDULE`
> v1 API call, using the ACL `GetMaintenanceSchedule` 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 89d0790fd5fea59e74276f462581fe0073594732 
>   src/tests/api_tests.cpp 91b3473452b8e65cab9f2e873837d64a0edf4b54 
> 
> 
> Diff: https://reviews.apache.org/r/59100/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 59100: Enabled authorization for v1 API call GET_MAINTENANCE_SCHEDULE.

2017-06-02 Thread Alexander Rojas 

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

(Updated June 2, 2017, 2:39 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_SCHEDULE`
v1 API call, using the ACL `GetMaintenanceSchedule` and the action
of the same name as the API call.

It also updates the ApiTests to take into account the authorization
case.


Diffs (updated)
-

  src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
  src/tests/api_tests.cpp 91b3473452b8e65cab9f2e873837d64a0edf4b54 


Diff: https://reviews.apache.org/r/59100/diff/5/

Changes: https://reviews.apache.org/r/59100/diff/4-5/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 59100: Enabled authorization for v1 API call GET_MAINTENANCE_SCHEDULE.

2017-06-02 Thread Alexander Rojas 


> On June 2, 2017, 3:43 a.m., Greg Mann wrote:
> > src/master/http.cpp
> > Lines 4222-4240 (patched)
> > 
> >
> > Simply copying into a new `Schedule` message is inefficient, but 
> > probably OK for now, as it's more readable and the output of this endpoint 
> > may not become too large.
> > 
> > I'm mostly just leaving this comment for posterity, to note that we may 
> > want to use the more sophisiticated JSON writer-based filtering in the 
> > future if performance becomes an issue, as we do in the `/state` handler.

I thought the same when I built it, but then I saw other related functons, like 
`getMaintenanceStatus` do something similar.


- Alexander


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


On June 1, 2017, 4:57 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59100/
> ---
> 
> (Updated June 1, 2017, 4:57 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_SCHEDULE`
> v1 API call, using the ACL `GetMaintenanceSchedule` 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 7060b8fa53e0682681c50e051908ffbbf50fb7da 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
>   src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09 
> 
> 
> Diff: https://reviews.apache.org/r/59100/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 59100: Enabled authorization for v1 API call GET_MAINTENANCE_SCHEDULE.

2017-06-01 Thread Greg Mann 

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




src/master/http.cpp
Lines 4148-4151 (patched)


See Till's comment on previous RR regarding spacing here, to make it 
consistent with the rest of this file.



src/master/http.cpp
Lines 4172-4173 (original), 4186-4194 (patched)


Why not use `defer` here, instead of the double lambda? Something like:

```
return approver.then(defer(
master->self(),
[=](Owned approver) -> Future {
  const mesos::maintenance::Schedule schedule =
_getMaintenanceSchedule(approver);

  return OK(JSON::protobuf(schedule), request.url.query.get("jsonp"));
},
lambda::_1));
```



src/master/http.cpp
Lines 4222-4240 (patched)


Simply copying into a new `Schedule` message is inefficient, but probably 
OK for now, as it's more readable and the output of this endpoint may not 
become too large.

I'm mostly just leaving this comment for posterity, to note that we may 
want to use the more sophisiticated JSON writer-based filtering in the future 
if performance becomes an issue, as we do in the `/state` handler.



src/master/http.cpp
Lines 4331-4337 (original), 4383-4394 (patched)


Ditto here regarding `defer`.



src/tests/api_tests.cpp
Lines 1263-1268 (patched)


Ditto here, regarding changing from this callback-based pattern to the 
standard `AWAIT_EXPECT_RESPONSE_STATUS_EQ` pattern.


- Greg Mann


On June 1, 2017, 2:57 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59100/
> ---
> 
> (Updated June 1, 2017, 2:57 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_SCHEDULE`
> v1 API call, using the ACL `GetMaintenanceSchedule` 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 7060b8fa53e0682681c50e051908ffbbf50fb7da 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
>   src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09 
> 
> 
> Diff: https://reviews.apache.org/r/59100/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 59100: Enabled authorization for v1 API call GET_MAINTENANCE_SCHEDULE.

2017-05-24 Thread Alexander Rojas 

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

(Updated May 24, 2017, 3:25 p.m.)


Review request for mesos, Adam B and Greg Mann.


Bugs: MESOS-7415
https://issues.apache.org/jira/browse/MESOS-7415


Repository: mesos


Description (updated)
---

Enables the use of authorization for the `GET_MAINTENANCE_SCHEDULE`
v1 API call, using the ACL `GetMaintenanceSchedule` and the action
of the same name as the API call.

It also updates the ApiTests to take into account the authorization
case.


Diffs (updated)
-

  src/master/http.cpp 7060b8fa53e0682681c50e051908ffbbf50fb7da 
  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
  src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09 


Diff: https://reviews.apache.org/r/59100/diff/4/

Changes: https://reviews.apache.org/r/59100/diff/3-4/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 59100: Enabled authorization for v1 API call GET_MAINTENANCE_SCHEDULE.

2017-05-22 Thread Alexander Rojas 

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

(Updated May 22, 2017, 6:11 p.m.)


Review request for mesos, Adam B and Greg Mann.


Bugs: MESOS-7415
https://issues.apache.org/jira/browse/MESOS-7415


Repository: mesos


Description (updated)
---

Enables the use of authorization for the `GET_MAINTENANCE_SCHEDULE`
v1 API call, using the ACL `GetMaintenanceSchedule` and the action
of the same name as the API call.

It also updates the `ApiTests` to take into account the authorization
case.


Diffs (updated)
-

  src/master/http.cpp 7060b8fa53e0682681c50e051908ffbbf50fb7da 
  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
  src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09 


Diff: https://reviews.apache.org/r/59100/diff/3/

Changes: https://reviews.apache.org/r/59100/diff/2-3/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 59100: Enabled authorization for v1 API call GET_MAINTENANCE_SCHEDULE.

2017-05-12 Thread Alexander Rojas 

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

(Updated May 12, 2017, 4:24 p.m.)


Review request for mesos, Adam B and Greg Mann.


Bugs: MESOS-7415
https://issues.apache.org/jira/browse/MESOS-7415


Repository: mesos


Description (updated)
---

Enables the use of authorization for the `GET_MAINTENANCE_SCHEDULE`
v1 API call, using the ACL `GetMaintenanceSchedule` and the action
of the same name as the API call.

It also updates the ApiTests to take into account the authorization
case.


Diffs (updated)
-

  src/master/http.cpp e2590a17044ac019b24a24629428d4ec8adc0c31 
  src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09 


Diff: https://reviews.apache.org/r/59100/diff/2/

Changes: https://reviews.apache.org/r/59100/diff/1-2/


Testing
---

make check


Thanks,

Alexander Rojas

Review Request 59100: Enabled authorization for v1 API call GET_MAINTENANCE_SCHEDULE.

2017-05-09 Thread Alexander Rojas 

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

Review request for mesos, Adam B and Greg Mann.


Bugs: MESOS-7415
https://issues.apache.org/jira/browse/MESOS-7415


Repository: mesos


Description
---

Enables the use of authorization for the `GET_MAINTENANCE_SCHEDULE`
v1 API call, using the ACL `GetMaintenanceSchedule` 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 e2590a17044ac019b24a24629428d4ec8adc0c31 
  src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09 


Diff: https://reviews.apache.org/r/59100/diff/1/


Testing
---

make check


Thanks,

Alexander Rojas