Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-06 Thread Jan Schlicht

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

(Updated Jan. 6, 2016, 11:26 a.m.)


Review request for mesos, Alexander Rukletsov and Till Toenshoff.


Changes
---

Addressed issues and merged tests.


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


Repository: mesos


Description
---

Quota Authorization: Added tests for quota removal authorization.


Diffs (updated)
-

  src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
  src/tests/master_quota_tests.cpp 81f0386d072d0176cb28eb229b274e424004f54c 

Diff: https://reviews.apache.org/r/41550/diff/


Testing
---

make check


Thanks,

Jan Schlicht



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-06 Thread Jan Schlicht


> On Jan. 5, 2016, 7:51 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, lines 1308-1318
> > 
> >
> > You don't really need an agent here, you can set the force flag and 
> > remove this boilerplate code. Check the cleanup here: 
> > https://reviews.apache.org/r/41939
> 
> Jan Schlicht wrote:
> This is great! But I have some concerns that are more review related. I 
> would need to add the same comments as you do in the cleanup. As your code is 
> currently being reviewed, these comments may change and I would need to 
> change them as well, or vice versa. Therefore I think it would be better to 
> only fix this in your cleanup and leave it like that for now. Do you agree? 
> I'm mostly concerned about the comments explaining why the force flag is set 
> for the HTTP requests.

Talked with my Shepherd about this. Let's remove the boilerplate code and add 
the comments. If these change in your review, you'd need to change them here as 
well, okay?


- Jan


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


On Jan. 6, 2016, 11:26 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41550/
> ---
> 
> (Updated Jan. 6, 2016, 11:26 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-4083
> https://issues.apache.org/jira/browse/MESOS-4083
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota Authorization: Added tests for quota removal authorization.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
>   src/tests/master_quota_tests.cpp 81f0386d072d0176cb28eb229b274e424004f54c 
> 
> Diff: https://reviews.apache.org/r/41550/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-06 Thread Alexander Rukletsov


> On Jan. 5, 2016, 6:51 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, lines 1308-1318
> > 
> >
> > You don't really need an agent here, you can set the force flag and 
> > remove this boilerplate code. Check the cleanup here: 
> > https://reviews.apache.org/r/41939
> 
> Jan Schlicht wrote:
> This is great! But I have some concerns that are more review related. I 
> would need to add the same comments as you do in the cleanup. As your code is 
> currently being reviewed, these comments may change and I would need to 
> change them as well, or vice versa. Therefore I think it would be better to 
> only fix this in your cleanup and leave it like that for now. Do you agree? 
> I'm mostly concerned about the comments explaining why the force flag is set 
> for the HTTP requests.
> 
> Jan Schlicht wrote:
> Talked with my Shepherd about this. Let's remove the boilerplate code and 
> add the comments. If these change in your review, you'd need to change them 
> here as well, okay?

Sure!


- Alexander


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


On Jan. 6, 2016, 10:53 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41550/
> ---
> 
> (Updated Jan. 6, 2016, 10:53 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-4083
> https://issues.apache.org/jira/browse/MESOS-4083
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota Authorization: Added tests for quota removal authorization.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
>   src/tests/master_quota_tests.cpp 81f0386d072d0176cb28eb229b274e424004f54c 
> 
> Diff: https://reviews.apache.org/r/41550/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-06 Thread Jan Schlicht


> On Jan. 5, 2016, 1:59 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, line 1380
> > 
> >
> > Again, I think this one can be merged with 
> > `AuthorizedQuotaRemoveRequest` (you can use scopes for clarity). After a 
> > second thought I think we can go even further and merge 
> > `UnauthorizedQuotaSetRequest` into those as well. What do you think?
> 
> Jan Schlicht wrote:
> In contrast to the other suggestions, I'm not sure about this one: To 
> merge this test with `AuthorizedQuotaRemoveRequest` or 
> `AuthorizedQuotasSetRequest`, both tests would need a much more sophisticated 
> ACL to be able to do this, because the ACLs can't be changed after the master 
> has been started. Currently we would need another credential, similar to 
> `DEFAULT_CREDENTIAL`, to support this. In my opinion this would be too much 
> changes needed.
> 
> Alexander Rukletsov wrote:
> I think having sophisticated ACL is not an issue, it may even be good for 
> testing purposes : ). I agree with your second argument though. Luckily, Greg 
> Mann recently posted a review which solves exactly this problem: 
> https://reviews.apache.org/r/41934/. I'm okay with committing this as is 
> right now, but I would love to see this revisited once r/41934 lands.

Greg's `DEFAULT_CREDENTIAL_2` just landed. Will merge these tests.


- Jan


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


On Jan. 5, 2016, 4:13 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41550/
> ---
> 
> (Updated Jan. 5, 2016, 4:13 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-4083
> https://issues.apache.org/jira/browse/MESOS-4083
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota Authorization: Added tests for quota removal authorization.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
>   src/tests/master_quota_tests.cpp 2f1bc3ae6a370e466f7cea9b597f51d7eccb1b33 
> 
> Diff: https://reviews.apache.org/r/41550/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-06 Thread Jan Schlicht

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

(Updated Jan. 6, 2016, 11:53 a.m.)


Review request for mesos, Alexander Rukletsov and Till Toenshoff.


Changes
---

Use force flag to remove the need for an agent.


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


Repository: mesos


Description
---

Quota Authorization: Added tests for quota removal authorization.


Diffs (updated)
-

  src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
  src/tests/master_quota_tests.cpp 81f0386d072d0176cb28eb229b274e424004f54c 

Diff: https://reviews.apache.org/r/41550/diff/


Testing
---

make check


Thanks,

Jan Schlicht



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-06 Thread Alexander Rukletsov

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



src/tests/master_quota_tests.cpp (line 1216)


s/acl/acl1



src/tests/master_quota_tests.cpp (lines 1239 - 1241)


This comment is related to the whole block and not to `Resources` struct 
only. Let's split it into two parts: one for the block and one for resources.



src/tests/master_quota_tests.cpp (lines 1257 - 1259)


Same here.



src/tests/master_quota_tests.cpp (lines 1291 - 1294)


Also here, and below. Since each block is actually a test case, we should 
prepend it with a comment explaining the intention behind, same as we do for 
each test.



src/tests/master_quota_tests.cpp (lines 1206 - 1207)


Now you also check that unauthorized princiapls can't do that. Please 
update the comment (and maybe the test name) accordingly (e.g. 
`AuthorizeQuotaRequests`).



src/tests/master_quota_tests.cpp (line 1329)


To be consistent with the previous test, let's 
s/AuthorizedQuotaRequestsWithoutPrincipal/AuthorizeQuotaRequestsWithoutPrincipal



src/tests/master_quota_tests.cpp (lines 1336 - 1342)


Blank lines, please.



src/tests/master_quota_tests.cpp (lines 1352 - 1364)


We agreed to wrap those in blocks, no? Same for remove quota below.


- Alexander Rukletsov


On Jan. 6, 2016, 10:53 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41550/
> ---
> 
> (Updated Jan. 6, 2016, 10:53 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-4083
> https://issues.apache.org/jira/browse/MESOS-4083
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota Authorization: Added tests for quota removal authorization.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
>   src/tests/master_quota_tests.cpp 81f0386d072d0176cb28eb229b274e424004f54c 
> 
> Diff: https://reviews.apache.org/r/41550/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-06 Thread Alexander Rukletsov

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



src/tests/master_quota_tests.cpp (line 1337)


s/acl/acl1


- Alexander Rukletsov


On Jan. 6, 2016, 10:53 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41550/
> ---
> 
> (Updated Jan. 6, 2016, 10:53 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-4083
> https://issues.apache.org/jira/browse/MESOS-4083
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota Authorization: Added tests for quota removal authorization.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
>   src/tests/master_quota_tests.cpp 81f0386d072d0176cb28eb229b274e424004f54c 
> 
> Diff: https://reviews.apache.org/r/41550/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-06 Thread Jan Schlicht


> On Jan. 5, 2016, 7:51 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, lines 1308-1318
> > 
> >
> > You don't really need an agent here, you can set the force flag and 
> > remove this boilerplate code. Check the cleanup here: 
> > https://reviews.apache.org/r/41939

This is great! But I have some concerns that are more review related. I would 
need to add the same comments as you do in the cleanup. As your code is 
currently being reviewed, these comments may change and I would need to change 
them as well, or vice versa. Therefore I think it would be better to only fix 
this in your cleanup and leave it like that for now. Do you agree? I'm mostly 
concerned about the comments explaining why the force flag is set for the HTTP 
requests.


- Jan


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


On Jan. 5, 2016, 4:13 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41550/
> ---
> 
> (Updated Jan. 5, 2016, 4:13 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-4083
> https://issues.apache.org/jira/browse/MESOS-4083
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota Authorization: Added tests for quota removal authorization.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
>   src/tests/master_quota_tests.cpp 2f1bc3ae6a370e466f7cea9b597f51d7eccb1b33 
> 
> Diff: https://reviews.apache.org/r/41550/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-06 Thread Alexander Rukletsov

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

Ship it!


Thanks for holding on till the end, almost there!


src/tests/master_quota_tests.cpp (line 1206)


s/and that/while



src/tests/master_quota_tests.cpp (line 1243)


There is no agent any more, right : )? How about
"A request can contain any amount of resources because we set the force 
flag."



src/tests/master_quota_tests.cpp (line 1261)


Same here.



src/tests/master_quota_tests.cpp (lines 1355 - 1357)


Please prepend the block with a comment as discussed in the previous review.



src/tests/master_quota_tests.cpp (lines 1373 - 1375)


Move the comment before the block, please


- Alexander Rukletsov


On Jan. 6, 2016, 1:15 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41550/
> ---
> 
> (Updated Jan. 6, 2016, 1:15 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-4083
> https://issues.apache.org/jira/browse/MESOS-4083
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota Authorization: Added tests for quota removal authorization.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
>   src/tests/master_quota_tests.cpp 81f0386d072d0176cb28eb229b274e424004f54c 
> 
> Diff: https://reviews.apache.org/r/41550/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-06 Thread Till Toenshoff

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

Ship it!


Ship It!

- Till Toenshoff


On Jan. 6, 2016, 1:15 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41550/
> ---
> 
> (Updated Jan. 6, 2016, 1:15 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-4083
> https://issues.apache.org/jira/browse/MESOS-4083
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota Authorization: Added tests for quota removal authorization.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
>   src/tests/master_quota_tests.cpp 81f0386d072d0176cb28eb229b274e424004f54c 
> 
> Diff: https://reviews.apache.org/r/41550/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-06 Thread Jan Schlicht

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

(Updated Jan. 6, 2016, 2:15 p.m.)


Review request for mesos, Alexander Rukletsov and Till Toenshoff.


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

Quota Authorization: Added tests for quota removal authorization.


Diffs (updated)
-

  src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
  src/tests/master_quota_tests.cpp 81f0386d072d0176cb28eb229b274e424004f54c 

Diff: https://reviews.apache.org/r/41550/diff/


Testing
---

make check


Thanks,

Jan Schlicht



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-05 Thread Mesos ReviewBot

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


Bad patch!

Reviews applied: [41546]

Failed command: ./support/apply-review.sh -n -r 41546

Error:
 2016-01-05 16:51:32 URL:https://reviews.apache.org/r/41546/diff/raw/ [946/946] 
-> "41546.patch" [1]
No files to lint

Error: Commit message summary (the first line) must start with a capital letter.

- Mesos ReviewBot


On Jan. 5, 2016, 3:13 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41550/
> ---
> 
> (Updated Jan. 5, 2016, 3:13 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-4083
> https://issues.apache.org/jira/browse/MESOS-4083
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota Authorization: Added tests for quota removal authorization.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
>   src/tests/master_quota_tests.cpp 2f1bc3ae6a370e466f7cea9b597f51d7eccb1b33 
> 
> Diff: https://reviews.apache.org/r/41550/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-05 Thread Jan Schlicht


> On Jan. 5, 2016, 1:59 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, line 1259
> > 
> >
> > This test almost entirely includes `AuthorizedQuotaSetRequest`. See 
> > below my suggestion regarding merging these.

Good idea! This will avoid code-duplication. Will rename the test to 
`AuthorizedQuotaRequests` as well.


> On Jan. 5, 2016, 1:59 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, line 1322
> > 
> >
> > This test includes `AuthorizedQuotaSetRequestWithoutPrincipal` 
> > entirely. How about you simply add ACLs for remove quota and requestDelete 
> > invokation to that test and rename it accordingly? You may want to wrap 
> > requests in scopes for clarity.

See above. Test will be renamed accordingly.


> On Jan. 5, 2016, 1:59 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, line 1380
> > 
> >
> > Again, I think this one can be merged with 
> > `AuthorizedQuotaRemoveRequest` (you can use scopes for clarity). After a 
> > second thought I think we can go even further and merge 
> > `UnauthorizedQuotaSetRequest` into those as well. What do you think?

In contrast to the other suggestions, I'm not sure about this one: To merge 
this test with `AuthorizedQuotaRemoveRequest` or `AuthorizedQuotasSetRequest`, 
both tests would need a much more sophisticated ACL to be able to do this, 
because the ACLs can't be changed after the master has been started. Currently 
we would need another credential, similar to `DEFAULT_CREDENTIAL`, to support 
this. In my opinion this would be too much changes needed.


- Jan


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


On Jan. 5, 2016, 12:17 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41550/
> ---
> 
> (Updated Jan. 5, 2016, 12:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-4083
> https://issues.apache.org/jira/browse/MESOS-4083
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota Authorization: Added tests for quota removal authorization.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
>   src/tests/master_quota_tests.cpp 2f1bc3ae6a370e466f7cea9b597f51d7eccb1b33 
> 
> Diff: https://reviews.apache.org/r/41550/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-05 Thread Jan Schlicht

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

(Updated Jan. 5, 2016, 4:13 p.m.)


Review request for mesos, Alexander Rukletsov and Till Toenshoff.


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

Quota Authorization: Added tests for quota removal authorization.


Diffs (updated)
-

  src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
  src/tests/master_quota_tests.cpp 2f1bc3ae6a370e466f7cea9b597f51d7eccb1b33 

Diff: https://reviews.apache.org/r/41550/diff/


Testing
---

make check


Thanks,

Jan Schlicht



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-05 Thread Alexander Rukletsov


> On Jan. 5, 2016, 4:51 p.m., Mesos ReviewBot wrote:
> > Bad patch!
> > 
> > Reviews applied: [41546]
> > 
> > Failed command: ./support/apply-review.sh -n -r 41546
> > 
> > Error:
> >  2016-01-05 16:51:32 URL:https://reviews.apache.org/r/41546/diff/raw/ 
> > [946/946] -> "41546.patch" [1]
> > No files to lint
> > 
> > Error: Commit message summary (the first line) must start with a capital 
> > letter.
> 
> Till Toenshoff wrote:
> Seems this pattern of doing enumerated chains isnt working well with our 
> commit hooks.

That's a new hook, right. Also, since we try to keep summary to 72 chars max, 
it makes sense to get rid of enumerating and long prefixes.


- Alexander


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


On Jan. 5, 2016, 3:13 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41550/
> ---
> 
> (Updated Jan. 5, 2016, 3:13 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-4083
> https://issues.apache.org/jira/browse/MESOS-4083
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota Authorization: Added tests for quota removal authorization.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
>   src/tests/master_quota_tests.cpp 2f1bc3ae6a370e466f7cea9b597f51d7eccb1b33 
> 
> Diff: https://reviews.apache.org/r/41550/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-05 Thread Till Toenshoff


> On Jan. 5, 2016, 4:51 p.m., Mesos ReviewBot wrote:
> > Bad patch!
> > 
> > Reviews applied: [41546]
> > 
> > Failed command: ./support/apply-review.sh -n -r 41546
> > 
> > Error:
> >  2016-01-05 16:51:32 URL:https://reviews.apache.org/r/41546/diff/raw/ 
> > [946/946] -> "41546.patch" [1]
> > No files to lint
> > 
> > Error: Commit message summary (the first line) must start with a capital 
> > letter.

Seems this pattern of doing enumerated chains isnt working well with our commit 
hooks.


- Till


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


On Jan. 5, 2016, 3:13 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41550/
> ---
> 
> (Updated Jan. 5, 2016, 3:13 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-4083
> https://issues.apache.org/jira/browse/MESOS-4083
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota Authorization: Added tests for quota removal authorization.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
>   src/tests/master_quota_tests.cpp 2f1bc3ae6a370e466f7cea9b597f51d7eccb1b33 
> 
> Diff: https://reviews.apache.org/r/41550/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-05 Thread Till Toenshoff


> On Jan. 5, 2016, 6:51 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, lines 1118-1124
> > 
> >
> > Let's insert blank lines between ACL blocks for readability.

Had the same thought while reviewing this.


- Till


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


On Jan. 5, 2016, 3:13 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41550/
> ---
> 
> (Updated Jan. 5, 2016, 3:13 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-4083
> https://issues.apache.org/jira/browse/MESOS-4083
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota Authorization: Added tests for quota removal authorization.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
>   src/tests/master_quota_tests.cpp 2f1bc3ae6a370e466f7cea9b597f51d7eccb1b33 
> 
> Diff: https://reviews.apache.org/r/41550/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-05 Thread Till Toenshoff

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



src/tests/authorization_tests.cpp (line 765)


As always, please describe the test in a few words  within a leading 
comment.



src/tests/authorization_tests.cpp (lines 826 - 828)


Very verbose but to me also very helpful comment.



src/tests/master_quota_tests.cpp (line 1119)


Blank line?



src/tests/master_quota_tests.cpp (line 1122)


Blank line?


- Till Toenshoff


On Jan. 5, 2016, 3:13 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41550/
> ---
> 
> (Updated Jan. 5, 2016, 3:13 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-4083
> https://issues.apache.org/jira/browse/MESOS-4083
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota Authorization: Added tests for quota removal authorization.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
>   src/tests/master_quota_tests.cpp 2f1bc3ae6a370e466f7cea9b597f51d7eccb1b33 
> 
> Diff: https://reviews.apache.org/r/41550/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-05 Thread Alexander Rukletsov

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



src/tests/master_quota_tests.cpp (lines 1118 - 1124)


Let's insert blank lines between ACL blocks for readability.



src/tests/master_quota_tests.cpp (lines 1201 - 1207)


Same here, let's add blank lines.



src/tests/master_quota_tests.cpp (lines 1308 - 1318)


You don't really need an agent here, you can set the force flag and remove 
this boilerplate code. Check the cleanup here: 
https://reviews.apache.org/r/41939



src/tests/master_quota_tests.cpp (lines 1320 - 1329)


You can wrap this code into a {} block for readability.



src/tests/master_quota_tests.cpp (lines 1332 - 1339)


Same here, a good candidate for a block.


- Alexander Rukletsov


On Jan. 5, 2016, 3:13 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41550/
> ---
> 
> (Updated Jan. 5, 2016, 3:13 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-4083
> https://issues.apache.org/jira/browse/MESOS-4083
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota Authorization: Added tests for quota removal authorization.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
>   src/tests/master_quota_tests.cpp 2f1bc3ae6a370e466f7cea9b597f51d7eccb1b33 
> 
> Diff: https://reviews.apache.org/r/41550/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-05 Thread Alexander Rukletsov


> On Jan. 5, 2016, 12:59 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, line 1380
> > 
> >
> > Again, I think this one can be merged with 
> > `AuthorizedQuotaRemoveRequest` (you can use scopes for clarity). After a 
> > second thought I think we can go even further and merge 
> > `UnauthorizedQuotaSetRequest` into those as well. What do you think?
> 
> Jan Schlicht wrote:
> In contrast to the other suggestions, I'm not sure about this one: To 
> merge this test with `AuthorizedQuotaRemoveRequest` or 
> `AuthorizedQuotasSetRequest`, both tests would need a much more sophisticated 
> ACL to be able to do this, because the ACLs can't be changed after the master 
> has been started. Currently we would need another credential, similar to 
> `DEFAULT_CREDENTIAL`, to support this. In my opinion this would be too much 
> changes needed.

I think having sophisticated ACL is not an issue, it may even be good for 
testing purposes : ). I agree with your second argument though. Luckily, Greg 
Mann recently posted a review which solves exactly this problem: 
https://reviews.apache.org/r/41934/. I'm okay with committing this as is right 
now, but I would love to see this revisited once r/41934 lands.


- Alexander


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


On Jan. 5, 2016, 3:13 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41550/
> ---
> 
> (Updated Jan. 5, 2016, 3:13 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-4083
> https://issues.apache.org/jira/browse/MESOS-4083
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota Authorization: Added tests for quota removal authorization.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
>   src/tests/master_quota_tests.cpp 2f1bc3ae6a370e466f7cea9b597f51d7eccb1b33 
> 
> Diff: https://reviews.apache.org/r/41550/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-05 Thread Jan Schlicht

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

(Updated Jan. 5, 2016, 12:17 p.m.)


Review request for mesos, Alexander Rukletsov and Till Toenshoff.


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

Quota Authorization: Added tests for quota removal authorization.


Diffs (updated)
-

  src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
  src/tests/master_quota_tests.cpp 2f1bc3ae6a370e466f7cea9b597f51d7eccb1b33 

Diff: https://reviews.apache.org/r/41550/diff/


Testing (updated)
---

make check


Thanks,

Jan Schlicht



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-05 Thread Alexander Rukletsov

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


After looking at all authz test I think we can make one `RequestAuthorization` 
test of 2 exisiting tests and 2 you propose. This will allow us to save on 
`SetUp()`, `TearDown()` and avoid copy-paste code. To separate cases inside the 
test you can scope them in {} blocks.


src/tests/master_quota_tests.cpp (line 1259)


This test almost entirely includes `AuthorizedQuotaSetRequest`. See below 
my suggestion regarding merging these.



src/tests/master_quota_tests.cpp (line 1322)


This test includes `AuthorizedQuotaSetRequestWithoutPrincipal` entirely. 
How about you simply add ACLs for remove quota and requestDelete invokation to 
that test and rename it accordingly? You may want to wrap requests in scopes 
for clarity.



src/tests/master_quota_tests.cpp (line 1380)


Again, I think this one can be merged with `AuthorizedQuotaRemoveRequest` 
(you can use scopes for clarity). After a second thought I think we can go even 
further and merge `UnauthorizedQuotaSetRequest` into those as well. What do you 
think?


- Alexander Rukletsov


On Jan. 5, 2016, 11:17 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41550/
> ---
> 
> (Updated Jan. 5, 2016, 11:17 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-4083
> https://issues.apache.org/jira/browse/MESOS-4083
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota Authorization: Added tests for quota removal authorization.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
>   src/tests/master_quota_tests.cpp 2f1bc3ae6a370e466f7cea9b597f51d7eccb1b33 
> 
> Diff: https://reviews.apache.org/r/41550/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>



Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.

2016-01-05 Thread Alexander Rukletsov


> On Dec. 28, 2015, 12:41 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, line 1072
> > 
> >
> > No need to introduce a variable if it used just once.
> 
> Jan Schlicht wrote:
> `EXPECT_EQ` does not like `DEFAULT_CREDENTIAL.principal()` as an argument 
> -- has to do with the way `DEFAULT_CREDENTIAL` is constructed. I will add a 
> comment to explain why the variable is introduced here.

Interesting. Okay!


- Alexander


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


On Jan. 5, 2016, 11:17 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41550/
> ---
> 
> (Updated Jan. 5, 2016, 11:17 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-4083
> https://issues.apache.org/jira/browse/MESOS-4083
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota Authorization: Added tests for quota removal authorization.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp 1d11a02032c142455debd9c78d4ee4cc6297a350 
>   src/tests/master_quota_tests.cpp 2f1bc3ae6a370e466f7cea9b597f51d7eccb1b33 
> 
> Diff: https://reviews.apache.org/r/41550/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>