----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63874/#review191688 -----------------------------------------------------------
Fix it, then Ship it! LGTM, a few nits below. bin/sentryCli Lines 21 (patched) <https://reviews.apache.org/r/63874/#comment269540> This can be written as export SENTRY_HOME=${SENTRY_HOME:-${MYHOME}} bin/sentryCli Lines 27 (patched) <https://reviews.apache.org/r/63874/#comment269541> Use [[ instead of [ bin/sentryCli Lines 32 (patched) <https://reviews.apache.org/r/63874/#comment269544> Use `[[ -z ${HADOOP_HOME} ]` instead to test for empty var bin/sentryCli Lines 38 (patched) <https://reviews.apache.org/r/63874/#comment269546> Use [[ bin/sentryCli Lines 45 (patched) <https://reviews.apache.org/r/63874/#comment269548> Use `+=` to append strings bin/sentryCli Lines 53 (patched) <https://reviews.apache.org/r/63874/#comment269551> Use `+=` sentry-tools/src/main/java/org/apache/sentry/shell/SentryCli.java Lines 117 (patched) <https://reviews.apache.org/r/63874/#comment269554> Why is the code parsing commands twice - here and above at line 104? sentry-tools/src/main/java/org/apache/sentry/shell/TopLevelShell.java Lines 49 (patched) <https://reviews.apache.org/r/63874/#comment269552> Nit: Use all caps for enums and don't put them on a single line. sentry-tools/src/main/java/org/apache/sentry/shell/TopLevelShell.java Lines 279 (patched) <https://reviews.apache.org/r/63874/#comment269555> Use switch() for comparing against enums. sentry-tools/src/main/java/org/apache/sentry/shell/TopLevelShell.java Lines 291 (patched) <https://reviews.apache.org/r/63874/#comment269556> Use switch() for comparing against enums - Alexander Kolbasov On Nov. 20, 2017, 3:05 p.m., Colm O hEigeartaigh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63874/ > ----------------------------------------------------------- > > (Updated Nov. 20, 2017, 3:05 p.m.) > > > Review request for sentry. > > > Bugs: SENTRY-1812 > https://issues.apache.org/jira/browse/SENTRY-1812 > > > Repository: sentry > > > Description > ------- > > A new interactive CLI to examine Sentry roles, groups, privileges, etc. > Extended Alexander's work to add support for Kafka, Solr, Sqoop, etc. > > > Diffs > ----- > > LICENSE.txt e6be7872 > bin/sentryCli PRE-CREATION > pom.xml d8636274 > sentry-dist/pom.xml 69f4fcca > sentry-tools/pom.xml PRE-CREATION > sentry-tools/src/main/java/org/apache/sentry/shell/GroupShell.java > PRE-CREATION > sentry-tools/src/main/java/org/apache/sentry/shell/PrivsShell.java > PRE-CREATION > sentry-tools/src/main/java/org/apache/sentry/shell/RolesShell.java > PRE-CREATION > sentry-tools/src/main/java/org/apache/sentry/shell/SentryCli.java > PRE-CREATION > sentry-tools/src/main/java/org/apache/sentry/shell/TopLevelShell.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/63874/diff/3/ > > > Testing > ------- > > Tested the CLI against the Sentry service. > > > Thanks, > > Colm O hEigeartaigh > >
