----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43026/#review117364 -----------------------------------------------------------
Fix it, then Ship it! code looks fine, with one comment. Seems like we shouldn't be building our own option parser though and instead should just use something like apache commons-cli sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentryShellHive.java (line 610) <https://reviews.apache.org/r/43026/#comment178515> Does this shell expect asserts fired as part of the error checking? If so, this won't work since asserts may not be enabled for prod code. - Lenni Kuff On Jan. 31, 2016, 3:31 a.m., Gregory Chanan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43026/ > ----------------------------------------------------------- > > (Updated Jan. 31, 2016, 3:31 a.m.) > > > Review request for sentry, Colin Ma and Sravya Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > SENTRY-749 and SENTRY-995 add tests that check for group options in a certain > order (e.g. [-lp List privilege, -rpr Revoke privilege from role, -lr List > role, -arg Add group to role, -drg Delete group from role, -gpr Grant > privilege to role, -dr Drop role, -cr Create role]), but the order is not > well defined because the org.apache.commons.cli.OptionGroup that is used for > the options stores them in a HashMap. > > So, instead of checking a defined order, we check that all the expected > options exist. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java > 354cf357dd3f74696de4d3eb49d707e980a1a641 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentryShellHive.java > 3907200d7da33faa038d63593a889f05303f7c18 > > Diff: https://reviews.apache.org/r/43026/diff/ > > > Testing > ------- > > Ran the unit tests on multiple machines / JVMs. > > > Thanks, > > Gregory Chanan > >
