> 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. > > Anne Yu wrote: > 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.
Yes, I'll add the comments, thanks. - Colin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31074/#review87993 ----------------------------------------------------------- On June 19, 2015, 1:36 a.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31074/ > ----------------------------------------------------------- > > (Updated June 19, 2015, 1:36 a.m.) > > > Review request for sentry and Anne Yu. > > > 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 > >
