> On Dec. 21, 2015, 11:20 p.m., Li Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java, > > line 61 > > <https://reviews.apache.org/r/34925/diff/2/?file=1165630#file1165630line61> > > > > should we add some log for the command == null case?
Thanks for point that, this will be checked by GnuParser.parse, and I also add the test case for missing command option. > On Dec. 21, 2015, 11:20 p.m., Li Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java, > > line 62 > > <https://reviews.apache.org/r/34925/diff/2/?file=1165630#file1165630line62> > > > > should we handle the case when requestorName.isEmpty() here? Thanks, the check is added. > On Dec. 21, 2015, 11:20 p.m., Li Li wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentryShellHive.java, > > line 404 > > <https://reviews.apache.org/r/34925/diff/2/?file=1165641#file1165641line404> > > > > The name of the test method here is not intuitive. It is not easy to > > get what is the difference between testNegativeCase1() and > > testNegativeCase2(). We'd better add some comments about the type of the > > covered negative tests. Thaks for point that, the method name is changed as testNegativeCaseWithInvalidArgument and testNegativeCaseWithoutRequiredArgument. > On Dec. 21, 2015, 11:20 p.m., Li Li wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentryShellHive.java, > > line 489 > > <https://reviews.apache.org/r/34925/diff/2/?file=1165641#file1165641line489> > > > > For the required argument test, it is also important that the error > > message returned is correct. > > For example, here the error msg should contains("Miss parameter for " + > > OPTION_DESC_CONF) Good point, I update the tests to verify the error message. > On Dec. 21, 2015, 11:20 p.m., Li Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java, > > line 145 > > <https://reviews.apache.org/r/34925/diff/2/?file=1165629#file1165629line145> > > > > how about adding a --help option to print out simpleShellOptions Thanks for point that, I'll add this option. - Colin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34925/#review111540 ----------------------------------------------------------- On Dec. 23, 2015, 8:42 a.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34925/ > ----------------------------------------------------------- > > (Updated Dec. 23, 2015, 8:42 a.m.) > > > Review request for sentry, Dapeng Sun, shen guoquan, and Prasad Mujumdar. > > > Repository: sentry > > > Description > ------- > > Create a simpler shell for sentry that handles command line arguments, eg > sentryShell --grant_role_privilege --role analyst --privilege > server=server1->database=db2->table=tab1->action=select > > > Diffs > ----- > > bin/sentryShell PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/Command.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/CommandUtil.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/CreateRoleCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/DropRoleCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/GrantPrivilegeToRoleCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/GrantRoleToGroupsCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/ListPrivilegesCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/ListRolesCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/RevokePrivilegeFromRoleCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/RevokeRoleFromGroupsCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentryShellHive.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java > 6bc9f75 > > Diff: https://reviews.apache.org/r/34925/diff/ > > > Testing > ------- > > > Thanks, > > Colin Ma > >
