You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Lenni Kuff <ls...@cloudera.com> on 2016/02/02 07:26:50 UTC
Re: Review Request 43026: SENTRY-1039: Sentry shell tests assume
order of option group privileges
-----------------------------------------------------------
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
>
>
Re: Review Request 43026: SENTRY-1039: Sentry shell tests assume
order of option group privileges
Posted by Gregory Chanan <gc...@cloudera.com>.
> On Feb. 2, 2016, 6:26 a.m., Lenni Kuff wrote:
> > 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
Thanks for the review, Lenni.
We are using commons-cli. Commons-cli uses a HashMap under the covers for storing the options groups, which is why you can't depend on the order.
- Gregory
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43026/#review117364
-----------------------------------------------------------
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
>
>