mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jan Schlicht" <...@mesosphere.io>
Subject Re: Review Request 41550: [5/5] Quota Authorization: Added tests for quota removal authorization.
Date Tue, 05 Jan 2016 15:02:27 GMT


> On Jan. 5, 2016, 1:59 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, line 1259
> > <https://reviews.apache.org/r/41550/diff/2/?file=1181807#file1181807line1259>
> >
> >     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
> > <https://reviews.apache.org/r/41550/diff/2/?file=1181807#file1181807line1322>
> >
> >     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
> > <https://reviews.apache.org/r/41550/diff/2/?file=1181807#file1181807line1380>
> >
> >     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
> 
>


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