----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40167/#review110641 -----------------------------------------------------------
include/mesos/authorizer/authorizer.proto (line 90) <https://reviews.apache.org/r/40167/#comment170654> I think "this principal" is a bit misleading, because we may specify multiple principals. I would say "a principal" or "listed principals" are a better fit. Not a native speaker though. include/mesos/authorizer/authorizer.proto (line 95) <https://reviews.apache.org/r/40167/#comment170657> Again, should not be singular, I suppose include/mesos/authorizer/authorizer.proto (line 96) <https://reviews.apache.org/r/40167/#comment170706> I see that you ensure this when you create requests in `authorizeCreateVolume`, but I'm not sure it's validated for ACLs. Do you think it makes sense to add validation in the `LocalAuthorizer::initialize()`? After a second thought, this note relates to the default implementation, because the master does not really validate it, right? Which means a 3rdparty authorizer may react to particular types. include/mesos/authorizer/authorizer.proto (lines 100 - 101) <https://reviews.apache.org/r/40167/#comment170655> Why did you wrap the comment this way? include/mesos/authorizer/authorizer.proto (line 106) <https://reviews.apache.org/r/40167/#comment170656> created the volume? - Alexander Rukletsov On Dec. 15, 2015, 9:18 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40167/ > ----------------------------------------------------------- > > (Updated Dec. 15, 2015, 9:18 p.m.) > > > Review request for mesos, Jie Yu, Michael Park, and Neil Conway. > > > Bugs: MESOS-4178 > https://issues.apache.org/jira/browse/MESOS-4178 > > > Repository: mesos > > > Description > ------- > > Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'. > > > Diffs > ----- > > include/mesos/authorizer/authorizer.proto > 74fcc86d3c92cb3aa27e45b647b1653705b3201c > > Diff: https://reviews.apache.org/r/40167/diff/ > > > Testing > ------- > > This is the second in a chain of 7 patches. `make check` was used to test > after all patches were applied. > > Note that this chain of patches touches many of the same files as another > chain beginning with Review #39985 and ending with Review #39989, which is > currently in review as well. To avoid conflicts, the beginning of this chain > begins on top of Review #39989. > > > Thanks, > > Greg Mann > >
