----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9771/#review17652 -----------------------------------------------------------
Hi Venkatesh, thank you very much for working on this and please accept my late review. The changes looks good me the except few nits highlighted below. I do have just one bigger concern regarding saved jobs. It seems to me that the code will load the password from file and store it inside the metastore, which might be seen as a security hole. What about explicitly not saving the password in metastore and instead loading it from the file each subsequent execution? src/docs/man/common-args.txt <https://reviews.apache.org/r/9771/#comment37457> Would you mind changing the argument to --password-file? It seems to be more consistent with the other parameters. src/docs/user/connecting.txt <https://reviews.apache.org/r/9771/#comment37458> I would recommend rephrasing that a bit. As the connectors are responsible for creating mapreduce job, they might override the default behavior. I would prefer to have this properly documented as it's about potential security hole. src/java/org/apache/sqoop/mapreduce/db/DBConfiguration.java <https://reviews.apache.org/r/9771/#comment37459> Can we put also login and entire JDBC url into the credentials object? src/java/org/apache/sqoop/tool/BaseSqoopTool.java <https://reviews.apache.org/r/9771/#comment37460> If you don't mind, I personally prefer explicit list of import classes than the wild card. - Jarek Cecho On March 6, 2013, 7:24 a.m., Seetharam Venkatesh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9771/ > ----------------------------------------------------------- > > (Updated March 6, 2013, 7:24 a.m.) > > > Review request for Sqoop and Jarek Cecho. > > > Description > ------- > > An attempt to add a way for users to pass credentials to sqoop in a secure > way by storing the password in a file under user's home directory with 400 > permissions. > > > This addresses bug https://issues.apache.org/jira/browse/SQOOP-914. > > https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/SQOOP-914 > > > Diffs > ----- > > src/docs/man/common-args.txt cf9c0c3 > src/docs/user/common-args.txt 0554f81 > src/docs/user/connecting.txt 44a5111 > src/docs/user/help.txt 24fbddc > src/docs/user/tools.txt 96bf777 > src/java/org/apache/sqoop/SqoopOptions.java addc889 > src/java/org/apache/sqoop/mapreduce/db/DBConfiguration.java d270bc8 > src/java/org/apache/sqoop/tool/BaseSqoopTool.java 684d4a5 > src/test/org/apache/sqoop/credentials/TestPassingSecurePassword.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/9771/diff/ > > > Testing > ------- > > Adds unit tests. All unit tests pass and checkstyle issues fixed. > > > Thanks, > > Seetharam Venkatesh > >
