Re: Review Request 57474: Added test for authorization of hierarchical roles.

2017-04-27 Thread Adam B 

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


Fix it, then Ship it!




LGTM. I will fix the tiny typo when I commit.


src/tests/authorization_tests.cpp
Line 1493 (original), 1493 (patched)


s/crete/create/


- Adam B


On April 18, 2017, 6:28 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57474/
> ---
> 
> (Updated April 18, 2017, 6:28 a.m.)
> 
> 
> Review request for mesos, Adam B and Benjamin Bannier.
> 
> 
> Bugs: MESOS-7026
> https://issues.apache.org/jira/browse/MESOS-7026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds tests for each of the actions which support hierarchical roles.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 3e18c70738b6b7098f37fadebb799a596e76452d 
> 
> 
> Diff: https://reviews.apache.org/r/57474/diff/5/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 57474: Added test for authorization of hierarchical roles.

2017-04-18 Thread Mesos Reviewbot 

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



Bad review!

Reviews applied: [57474, 57473, 58292, 57166, 56805, 57165, 57164]

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot


On April 18, 2017, 1:28 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57474/
> ---
> 
> (Updated April 18, 2017, 1:28 p.m.)
> 
> 
> Review request for mesos, Adam B and Benjamin Bannier.
> 
> 
> Bugs: MESOS-7026
> https://issues.apache.org/jira/browse/MESOS-7026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds tests for each of the actions which support hierarchical roles.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 3e18c70738b6b7098f37fadebb799a596e76452d 
> 
> 
> Diff: https://reviews.apache.org/r/57474/diff/5/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 57474: Added test for authorization of hierarchical roles.

2017-04-18 Thread Alexander Rojas 

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

(Updated April 18, 2017, 3:28 p.m.)


Review request for mesos, Adam B and Benjamin Bannier.


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


Repository: mesos


Description
---

Adds tests for each of the actions which support hierarchical roles.


Diffs (updated)
-

  src/tests/authorization_tests.cpp 3e18c70738b6b7098f37fadebb799a596e76452d 


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

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


Testing
---

`make check`


Thanks,

Alexander Rojas

Re: Review Request 57474: Added test for authorization of hierarchical roles.

2017-04-12 Thread Adam B 

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



Test logic looks good, although I question "archbishop" as a principal.
And it doesn't look like you thoroughly reviewed the text in your comments.


src/tests/authorization_tests.cpp
Lines 888 (patched)


s/Cheks/Checks/



src/tests/authorization_tests.cpp
Lines 912 (patched)


When has an archbishop ever also been a king? I was expecting another 
person's name. As a title, "archbishop" makes more sense as a Mesos role than a 
principal.

Archbishop William King was technically both.
https://en.wikipedia.org/wiki/William_King_(bishop)



src/tests/authorization_tests.cpp
Lines 1048 (patched)


s/Not not/Not/
s/irself/itself/



src/tests/authorization_tests.cpp
Lines 1184 (patched)


s/register frameworks/reserve resources/



src/tests/authorization_tests.cpp
Lines 1478 (patched)


"Not not" and "irself" again. Please fix throughout.



src/tests/authorization_tests.cpp
Lines 1597 (patched)


s/register frameworks/create volumes/



src/tests/authorization_tests.cpp
Lines 1866 (patched)


These could uses comments like the others



src/tests/authorization_tests.cpp
Line 1578 (original), 1943 (patched)


"register frameworks" again. Please fix throughout.



src/tests/authorization_tests.cpp
Lines 4522-4524 (patched)


This comment belongs above the previous block of archbishop->king



src/tests/authorization_tests.cpp
Lines 4588 (patched)


s/view any role/update the weight of any role/



src/tests/authorization_tests.cpp
Lines 4737 (patched)


s/get weights/get quotas/g and s/update weights/get quotas/g throughout 
this test.



src/tests/authorization_tests.cpp
Lines 4753 (patched)


s/update the weight/view the quota/



src/tests/authorization_tests.cpp
Lines 4760 (patched)


s/view any role/view quota for any role/


- Adam B


On April 11, 2017, 1:47 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57474/
> ---
> 
> (Updated April 11, 2017, 1:47 a.m.)
> 
> 
> Review request for mesos, Adam B and Benjamin Bannier.
> 
> 
> Bugs: MESOS-7026
> https://issues.apache.org/jira/browse/MESOS-7026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds tests for each of the actions which support hierarchical roles.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 3e18c70738b6b7098f37fadebb799a596e76452d 
> 
> 
> Diff: https://reviews.apache.org/r/57474/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 57474: Added test for authorization of hierarchical roles.

2017-04-12 Thread Adam B 


> On March 20, 2017, 10:28 a.m., Benjamin Bannier wrote:
> > LGTM but for one non-functional issue.
> > 
> > I believe it might make sense to combine this into one change set with the 
> > previous patch since we e.g., likely wouldn't be interested in backporting. 
> > Maybe @adam-mesos has some idea.
> 
> Alexander Rojas wrote:
> I'm inclined to agree with you. Other reviewers in the past have 
> requested to break patches betweent he functional changes and tests. Let's 
> @adam-mesos give us a directive here.

Either way's fine with me. It's nice having them in separate reviews so (as a 
reviewer) I can review all but the tests in my first pass, but I also do that 
based on filenames in a combined impl+tests patch. As committer, though I may 
stamp a ShipIt on the impl patch while still polishing up the test patch, I can 
still choose to wait to commit both in the same push.


- Adam


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


On April 11, 2017, 1:47 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57474/
> ---
> 
> (Updated April 11, 2017, 1:47 a.m.)
> 
> 
> Review request for mesos, Adam B and Benjamin Bannier.
> 
> 
> Bugs: MESOS-7026
> https://issues.apache.org/jira/browse/MESOS-7026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds tests for each of the actions which support hierarchical roles.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 3e18c70738b6b7098f37fadebb799a596e76452d 
> 
> 
> Diff: https://reviews.apache.org/r/57474/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 57474: Added test for authorization of hierarchical roles.

2017-04-11 Thread Alexander Rojas 

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

(Updated April 11, 2017, 1:47 a.m.)


Review request for mesos, Adam B and Benjamin Bannier.


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


Repository: mesos


Description
---

Adds tests for each of the actions which support hierarchical roles.


Diffs
-

  src/tests/authorization_tests.cpp 3e18c70738b6b7098f37fadebb799a596e76452d 


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


Testing
---

`make check`


Thanks,

Alexander Rojas

Re: Review Request 57474: Added test for authorization of hierarchical roles.

2017-04-04 Thread Mesos Reviewbot 

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



Bad review!

Reviews applied: [57474, 57473, 57166, 56805, 57165, 57164]

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot


On April 4, 2017, 9:22 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57474/
> ---
> 
> (Updated April 4, 2017, 9:22 a.m.)
> 
> 
> Review request for mesos, Adam B and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds tests for each of the actions which support hierarchical roles.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 3e18c70738b6b7098f37fadebb799a596e76452d 
> 
> 
> Diff: https://reviews.apache.org/r/57474/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 57474: Added test for authorization of hierarchical roles.

2017-04-04 Thread Alexander Rojas 

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

(Updated April 4, 2017, 11:22 a.m.)


Review request for mesos, Adam B and Benjamin Bannier.


Changes
---

Rebased.


Repository: mesos


Description
---

Adds tests for each of the actions which support hierarchical roles.


Diffs (updated)
-

  src/tests/authorization_tests.cpp 3e18c70738b6b7098f37fadebb799a596e76452d 


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

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


Testing
---

`make check`


Thanks,

Alexander Rojas

Re: Review Request 57474: Added test for authorization of hierarchical roles.

2017-03-21 Thread Mesos Reviewbot 

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



Bad review!

Reviews applied: [57474, 57473, 57472, 57166, 56805, 57165, 57164]

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot


On March 21, 2017, 3:21 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57474/
> ---
> 
> (Updated March 21, 2017, 3:21 p.m.)
> 
> 
> Review request for mesos, Adam B and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds tests for each of the actions which support hierarchical roles.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp cd15add7d7b01c2b316ac946e017a4d0b502237f 
> 
> 
> Diff: https://reviews.apache.org/r/57474/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 57474: Added test for authorization of hierarchical roles.

2017-03-21 Thread Alexander Rojas 

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

(Updated March 21, 2017, 4:21 p.m.)


Review request for mesos, Adam B and Benjamin Bannier.


Changes
---

Added missing comments.


Repository: mesos


Description
---

Adds tests for each of the actions which support hierarchical roles.


Diffs (updated)
-

  src/tests/authorization_tests.cpp cd15add7d7b01c2b316ac946e017a4d0b502237f 


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

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


Testing
---

`make check`


Thanks,

Alexander Rojas

Re: Review Request 57474: Added test for authorization of hierarchical roles.

2017-03-20 Thread Benjamin Bannier 

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



LGTM but for one non-functional issue.

I believe it might make sense to combine this into one change set with the 
previous patch since we e.g., likely wouldn't be interested in backporting. 
Maybe @adam-mesos has some idea.


src/tests/authorization_tests.cpp
Lines 1171 (patched)


The blocks you add here should probably be documented in the same fat style 
currently used in this file.

Here and elsewhere in this patch.


- Benjamin Bannier


On March 13, 2017, 4:35 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57474/
> ---
> 
> (Updated March 13, 2017, 4:35 p.m.)
> 
> 
> Review request for mesos, Adam B and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds tests for each of the actions which support hierarchical roles.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 42edecc794b71a00ca32d26ae9b74e9f3ef97510 
> 
> 
> Diff: https://reviews.apache.org/r/57474/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 57474: Added test for authorization of hierarchical roles.

2017-03-13 Thread Alexander Rojas 

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

(Updated March 13, 2017, 4:35 p.m.)


Review request for mesos, Adam B and Benjamin Bannier.


Repository: mesos


Description
---

Adds tests for each of the actions which support hierarchical roles.


Diffs
-

  src/tests/authorization_tests.cpp 42edecc794b71a00ca32d26ae9b74e9f3ef97510 


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


Testing
---

`make check`


Thanks,

Alexander Rojas

Re: Review Request 57474: Added test for authorization of hierarchical roles.

2017-03-13 Thread Mesos Reviewbot 

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



Bad review!

Reviews applied: [57474, 57473, 57472, 57166, 56805, 57165, 57164]

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot


On March 13, 2017, 3:18 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57474/
> ---
> 
> (Updated March 13, 2017, 3:18 p.m.)
> 
> 
> Review request for mesos, Adam B and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds tests for each of the actions which support hierarchical roles.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 42edecc794b71a00ca32d26ae9b74e9f3ef97510 
> 
> 
> Diff: https://reviews.apache.org/r/57474/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 57474: Added test for authorization of hierarchical roles.

2017-03-13 Thread Alexander Rojas 

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

(Updated March 13, 2017, 2:18 p.m.)


Review request for mesos, Adam B and Benjamin Bannier.


Changes
---

Applies bbannier fixups


Repository: mesos


Description
---

Adds tests for each of the actions which support hierarchical roles.


Diffs (updated)
-

  src/tests/authorization_tests.cpp 42edecc794b71a00ca32d26ae9b74e9f3ef97510 


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

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


Testing
---

`make check`


Thanks,

Alexander Rojas

Re: Review Request 57474: Added test for authorization of hierarchical roles.

2017-03-10 Thread Mesos Reviewbot 

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



Bad review!

Reviews applied: [57474, 57473, 57472, 57166, 56805, 57165, 57164]

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot


On March 9, 2017, 10:26 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57474/
> ---
> 
> (Updated March 9, 2017, 10:26 a.m.)
> 
> 
> Review request for mesos, Adam B and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds tests for each of the actions which support hierarchical roles.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 42edecc794b71a00ca32d26ae9b74e9f3ef97510 
> 
> 
> Diff: https://reviews.apache.org/r/57474/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 57474: Added test for authorization of hierarchical roles.

2017-03-10 Thread Benjamin Bannier 

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



Thanks for updating this, looks mostly good to me; I just hope the principals 
and roles you picked help everybody.

Overall issues I believe we should fix:

* We don't have existing tests for `UPDATE_WEIGHT` and `GET_QUOTA` we could 
update here. Since we add extra branching for these we should add tests now.
* This file uses a very verbose commenting style while most of the new code you 
added has no comments. I am not sure the way comments are used here is 
effective, but let's try to be more consistent.


src/tests/authorization_tests.cpp
Lines 907 (patched)


`archbishop`.

Here and everywhere else.


- Benjamin Bannier


On March 9, 2017, 7:26 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57474/
> ---
> 
> (Updated March 9, 2017, 7:26 p.m.)
> 
> 
> Review request for mesos, Adam B and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds tests for each of the actions which support hierarchical roles.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 42edecc794b71a00ca32d26ae9b74e9f3ef97510 
> 
> 
> Diff: https://reviews.apache.org/r/57474/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Review Request 57474: Added test for authorization of hierarchical roles.

2017-03-09 Thread Alexander Rojas 

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

Review request for mesos, Adam B and Benjamin Bannier.


Repository: mesos


Description
---

Adds tests for each of the actions which support hierarchical roles.


Diffs
-

  src/tests/authorization_tests.cpp 42edecc794b71a00ca32d26ae9b74e9f3ef97510 


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


Testing
---

`make check`


Thanks,

Alexander Rojas