> On June 15, 2015, 11:12 p.m., Anne Yu wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java, > > line 44 > > <https://reviews.apache.org/r/31074/diff/4/?file=976407#file976407line44> > > > > Seems like PRIVILIEGE1...8 is derrived from testPolicyImport.ini. Could > > it be possible to combine these two definitions together? In this case, > > when make change to the first place will automatically reflect to the file. > > It will be easier to maintain the code. > > Colin Ma wrote: > Thanks for the comments, currently, all test data for > TestPolicyImportExport are read from the .ini file, and the PRIVILIEGE1...8 > are used for verification. The PRIVILIEGE1...8 are duplicated with the > testPolicyImport.ini, to avoid this, maybe we can generate the > testPolicyImport.ini by PolicyFile, but to test the import/export with file, > it's better to use the testPolicyImport.ini as currently. For the code > maintain, we have to update the both place if anything changed, but I think > it's not a big problem for the test class.
IC, the implementation in this case gets more complicated. Sounds good to me. Could it be possible can add more comments either after PRIVILIEGE1...8 or in the ini files to explain they are correlated. It takes me some time to figure out their relationship by staring at the codes. - Anne ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31074/#review87993 ----------------------------------------------------------- On June 2, 2015, 8:38 a.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31074/ > ----------------------------------------------------------- > > (Updated June 2, 2015, 8:38 a.m.) > > > Review request for sentry. > > > Repository: sentry > > > Description > ------- > > Update SentryConfigTool for import/export feature > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java > 4388ca0 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java > 7ebc0e4 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java > PRE-CREATION > sentry-tests/sentry-tests-hive/src/test/resources/testPolicyImport.ini > PRE-CREATION > sentry-tests/sentry-tests-hive/src/test/resources/testPolicyImportAdmin.ini > PRE-CREATION > sentry-tests/sentry-tests-hive/src/test/resources/testPolicyImportError.ini > PRE-CREATION > > Diff: https://reviews.apache.org/r/31074/diff/ > > > Testing > ------- > > > Thanks, > > Colin Ma > >
