You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Colin Ma <ju...@intel.com> on 2015/03/09 03:52:29 UTC
Re: Review Request 31074: Sentry-657: Update SentryConfigTool for
import/export feature
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31074/
-----------------------------------------------------------
(Updated March 9, 2015, 2:52 a.m.)
Review request for sentry.
Repository: sentry
Description
-------
Update SentryConfigTool for import/export feature
Diffs (updated)
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java ecbd664
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
Re: Review Request 31074: Sentry-657: Update SentryConfigTool for
import/export feature
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31074/#review87940
-----------------------------------------------------------
Ship it!
Ship It!
- Sravya Tirukkovalur
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
>
>
Re: Review Request 31074: Sentry-657: Update SentryConfigTool for
import/export feature
Posted by Colin Ma <ju...@intel.com>.
> 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
>
>
Re: Review Request 31074: Sentry-657: Update SentryConfigTool for
import/export feature
Posted by Anne Yu <an...@cloudera.com>.
> 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
>
>
Re: Review Request 31074: Sentry-657: Update SentryConfigTool for
import/export feature
Posted by Colin Ma <ju...@intel.com>.
> 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.
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.
- Colin
-----------------------------------------------------------
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
>
>
Re: Review Request 31074: Sentry-657: Update SentryConfigTool for
import/export feature
Posted by Anne Yu <an...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31074/#review87993
-----------------------------------------------------------
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
<https://reviews.apache.org/r/31074/#comment140404>
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.
- Anne Yu
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
>
>
Re: Review Request 31074: Sentry-657: Update SentryConfigTool for
import/export feature
Posted by Anne Yu <an...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31074/#review88486
-----------------------------------------------------------
Ship it!
Ship It!
- Anne Yu
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
>
>
Re: Review Request 31074: Sentry-657: Update SentryConfigTool for
import/export feature
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 31074: Sentry-657: Update SentryConfigTool for
import/export feature
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 31074: Sentry-657: Update SentryConfigTool for
import/export feature
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31074/#review85686
-----------------------------------------------------------
Ship it!
Looks good to me, thanks Colin!
- Sravya Tirukkovalur
On May 20, 2015, 1:35 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31074/
> -----------------------------------------------------------
>
> (Updated May 20, 2015, 1:35 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 ecbd664
> 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
>
>
Re: Review Request 31074: Sentry-657: Update SentryConfigTool for
import/export feature
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31074/
-----------------------------------------------------------
(Updated May 20, 2015, 1:35 a.m.)
Review request for sentry.
Repository: sentry
Description
-------
Update SentryConfigTool for import/export feature
Diffs (updated)
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java ecbd664
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