> On Feb. 14, 2017, 1:11 a.m., Benjamin Mahler wrote: > > src/tests/master_validation_tests.cpp, lines 471-490 > > <https://reviews.apache.org/r/55462/diff/5/?file=1631983#file1631983line471> > > > > We could generalize this test to be: a framework (that has role `"*"` > > in its roles, e.g. `{role1, role2, *}`) cannot make a reservation to `*`. > > As it stands we seem to be missing the case where star is present but there > > are multiple roles.
There is some needed redundancy as the error messages for the different cases are not identical, and checking these helps avoid randomly catching any other validation we might introduce on the data hardcoded in this test at some later point. I was also thinking about `[*, ..]`, but ultimately decided against adding an additional check for that since it didn't really cover new branches in the originally proposed validation code; this seems even more true now where we do not explicitly handle an empty `roles` set or `role=*` anymore in the validation. Instead the code uses some general set of roles and this test would fail would that validation be e.g., removed. Do you think it would still make sense to add such a test for added redundancy? > On Feb. 14, 2017, 1:11 a.m., Benjamin Mahler wrote: > > src/tests/master_validation_tests.cpp, lines 493-513 > > <https://reviews.apache.org/r/55462/diff/5/?file=1631983#file1631983line493> > > > > This could be generalized to be making a reservation to a role that is > > not within its roles (which may be already caught by the mismatched > > allocation role since we expect allocation role == reserve role)? I added test cases for default role, empty roles, and some values for role or roles, and merged this with the previous test case. I made these inline, but can work to generate these instead (requires some extra legwork to generate differing error message which I believe would be worth it). Let me know having these generated instead of inlined makes more sense to you. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55462/#review165404 ----------------------------------------------------------- On Feb. 14, 2017, 2:47 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55462/ > ----------------------------------------------------------- > > (Updated Feb. 14, 2017, 2:47 a.m.) > > > Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu. > > > Bugs: MESOS-6730 > https://issues.apache.org/jira/browse/MESOS-6730 > > > Repository: mesos > > > Description > ------- > > This change introduces validation of the 'AllocationInfo' of resources > used in reservations. > > > Diffs > ----- > > src/master/validation.cpp 2bfca95713319a195e43e990cbc2dc5570b89c4e > src/tests/master_validation_tests.cpp > fd1f4a6cf0661351e265e50da1bd6ea04ed13d26 > > Diff: https://reviews.apache.org/r/55462/diff/ > > > Testing > ------- > > N/A yet. > > > Thanks, > > Benjamin Bannier > >
