You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Jarek Cecho <ja...@apache.org> on 2015/02/17 20:17:56 UTC
Review Request 31127: SQOOP-2109 Sqoop2: Shell module is including
test dependencies when building binary artifact
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31127/
-----------------------------------------------------------
Review request for Sqoop.
Bugs: SQOOP-2109
https://issues.apache.org/jira/browse/SQOOP-2109
Repository: sqoop-sqoop2
Description
-------
There has been several things that I had to fix in order to get this right:
* Shell packaging was pulling in all dependencies regardless of their scope, I had to manually switch it from "test" to "runtime"
* Common was directly depending on common-test without properly specifying testing scope
* Security module was completely missing dependency on com.bueust (that wasn't problem because 2) has pulled it in transitively)
Diffs
-----
common/pom.xml 9e593b2e23d54db34ad5737c08637365792e6537
pom.xml d48389ee60c1fbc5fa76e85a577d48f1e2032aec
security/pom.xml e4e61ccaa43aa62c7db66fe72ade3bc25b4155aa
shell/pom.xml e48c86a20f0bb5bf0ba8a5f12d563bb4c6f0f26a
Diff: https://reviews.apache.org/r/31127/diff/
Testing
-------
I've run the packaging target and the shell/lib directory size decreased from 46MB to 28MB (almost a half!), excluding all the testing dependencies.
Thanks,
Jarek Cecho
Re: Review Request 31127: SQOOP-2109 Sqoop2: Shell module is
including test dependencies when building binary artifact
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31127/#review72781
-----------------------------------------------------------
Ship it!
Ship It!
- Abraham Elmahrek
On Feb. 17, 2015, 7:57 p.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31127/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2015, 7:57 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2109
> https://issues.apache.org/jira/browse/SQOOP-2109
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> There has been several things that I had to fix in order to get this right:
>
> * Shell packaging was pulling in all dependencies regardless of their scope, I had to manually switch it from "test" to "runtime"
> * Common was directly depending on common-test without properly specifying testing scope
> * Security module was completely missing dependency on com.bueust (that wasn't problem because 2) has pulled it in transitively)
>
>
> Diffs
> -----
>
> common/pom.xml 9e593b2
> pom.xml d48389e
> security/pom.xml e4e61cc
> shell/pom.xml e48c86a
>
> Diff: https://reviews.apache.org/r/31127/diff/
>
>
> Testing
> -------
>
> I've run the packaging target and the shell/lib directory size decreased from 46MB to 28MB (almost a half!), excluding all the testing dependencies.
>
>
> Thanks,
>
> Jarek Cecho
>
>
Re: Review Request 31127: SQOOP-2109 Sqoop2: Shell module is
including test dependencies when building binary artifact
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31127/
-----------------------------------------------------------
(Updated Feb. 17, 2015, 7:57 p.m.)
Review request for Sqoop.
Changes
-------
I've moved the testng dependency to the root pom, so that all modules see it.
Bugs: SQOOP-2109
https://issues.apache.org/jira/browse/SQOOP-2109
Repository: sqoop-sqoop2
Description
-------
There has been several things that I had to fix in order to get this right:
* Shell packaging was pulling in all dependencies regardless of their scope, I had to manually switch it from "test" to "runtime"
* Common was directly depending on common-test without properly specifying testing scope
* Security module was completely missing dependency on com.bueust (that wasn't problem because 2) has pulled it in transitively)
Diffs (updated)
-----
common/pom.xml 9e593b2
pom.xml d48389e
security/pom.xml e4e61cc
shell/pom.xml e48c86a
Diff: https://reviews.apache.org/r/31127/diff/
Testing
-------
I've run the packaging target and the shell/lib directory size decreased from 46MB to 28MB (almost a half!), excluding all the testing dependencies.
Thanks,
Jarek Cecho
Re: Review Request 31127: SQOOP-2109 Sqoop2: Shell module is
including test dependencies when building binary artifact
Posted by Jarek Cecho <ja...@apache.org>.
> On Feb. 17, 2015, 7:49 p.m., Veena Basavaraj wrote:
> > Few comments.
> >
> > Patch is good.
> > 1. Please update steps in the JIRA or RB on how this was discovered or was it a serendepity:), so future reviews and testing of sqoop releases can use this methodology and not have to wait until multiple RCs, ( In future we could also automate the same methodology for quicker resolution)
> > 2. There are 3 separate issues, 1 and 2 are unreleated, though 1 and 3 are related, wondered if separate issues would have been nice.
1. I'm afraid that serendepity is the right answer here. I was looking for all the jars that we are shipping and accidentally noticed that we're shipping stuff that we should not.
2. You're right that are unrelated to some extent. I'm happy to cut them if needed.
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31127/#review72768
-----------------------------------------------------------
On Feb. 17, 2015, 7:57 p.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31127/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2015, 7:57 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2109
> https://issues.apache.org/jira/browse/SQOOP-2109
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> There has been several things that I had to fix in order to get this right:
>
> * Shell packaging was pulling in all dependencies regardless of their scope, I had to manually switch it from "test" to "runtime"
> * Common was directly depending on common-test without properly specifying testing scope
> * Security module was completely missing dependency on com.bueust (that wasn't problem because 2) has pulled it in transitively)
>
>
> Diffs
> -----
>
> common/pom.xml 9e593b2
> pom.xml d48389e
> security/pom.xml e4e61cc
> shell/pom.xml e48c86a
>
> Diff: https://reviews.apache.org/r/31127/diff/
>
>
> Testing
> -------
>
> I've run the packaging target and the shell/lib directory size decreased from 46MB to 28MB (almost a half!), excluding all the testing dependencies.
>
>
> Thanks,
>
> Jarek Cecho
>
>
Re: Review Request 31127: SQOOP-2109 Sqoop2: Shell module is
including test dependencies when building binary artifact
Posted by Veena Basavaraj <vb...@cloudera.com>.
> On Feb. 17, 2015, 11:49 a.m., Veena Basavaraj wrote:
> > Few comments.
> >
> > Patch is good.
> > 1. Please update steps in the JIRA or RB on how this was discovered or was it a serendepity:), so future reviews and testing of sqoop releases can use this methodology and not have to wait until multiple RCs, ( In future we could also automate the same methodology for quicker resolution)
> > 2. There are 3 separate issues, 1 and 2 are unreleated, though 1 and 3 are related, wondered if separate issues would have been nice.
>
> Jarek Cecho wrote:
> 1. I'm afraid that serendepity is the right answer here. I was looking for all the jars that we are shipping and accidentally noticed that we're shipping stuff that we should not.
> 2. You're right that are unrelated to some extent. I'm happy to cut them if needed.
would be nice if we split it up #2. and also update license since this release atleast I updated the LICENSE.txt with relevant jars and licenses that were added since the last 1.99.4 release.
For the previous ones, happy to create a new one, so we can audit the rest of the jars and update licensens ( so this is not a blocker for older jars )
to track testing of #1, created a new JIRA as we discussed offline.
- Veena
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31127/#review72768
-----------------------------------------------------------
On Feb. 17, 2015, 11:57 a.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31127/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2015, 11:57 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2109
> https://issues.apache.org/jira/browse/SQOOP-2109
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> There has been several things that I had to fix in order to get this right:
>
> * Shell packaging was pulling in all dependencies regardless of their scope, I had to manually switch it from "test" to "runtime"
> * Common was directly depending on common-test without properly specifying testing scope
> * Security module was completely missing dependency on com.bueust (that wasn't problem because 2) has pulled it in transitively)
>
>
> Diffs
> -----
>
> common/pom.xml 9e593b2
> pom.xml d48389e
> security/pom.xml e4e61cc
> shell/pom.xml e48c86a
>
> Diff: https://reviews.apache.org/r/31127/diff/
>
>
> Testing
> -------
>
> I've run the packaging target and the shell/lib directory size decreased from 46MB to 28MB (almost a half!), excluding all the testing dependencies.
>
>
> Thanks,
>
> Jarek Cecho
>
>
Re: Review Request 31127: SQOOP-2109 Sqoop2: Shell module is
including test dependencies when building binary artifact
Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31127/#review72768
-----------------------------------------------------------
Few comments.
Patch is good.
1. Please update steps in the JIRA or RB on how this was discovered or was it a serendepity:), so future reviews and testing of sqoop releases can use this methodology and not have to wait until multiple RCs, ( In future we could also automate the same methodology for quicker resolution)
2. There are 3 separate issues, 1 and 2 are unreleated, though 1 and 3 are related, wondered if separate issues would have been nice.
- Veena Basavaraj
On Feb. 17, 2015, 11:17 a.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31127/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2015, 11:17 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2109
> https://issues.apache.org/jira/browse/SQOOP-2109
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> There has been several things that I had to fix in order to get this right:
>
> * Shell packaging was pulling in all dependencies regardless of their scope, I had to manually switch it from "test" to "runtime"
> * Common was directly depending on common-test without properly specifying testing scope
> * Security module was completely missing dependency on com.bueust (that wasn't problem because 2) has pulled it in transitively)
>
>
> Diffs
> -----
>
> common/pom.xml 9e593b2e23d54db34ad5737c08637365792e6537
> pom.xml d48389ee60c1fbc5fa76e85a577d48f1e2032aec
> security/pom.xml e4e61ccaa43aa62c7db66fe72ade3bc25b4155aa
> shell/pom.xml e48c86a20f0bb5bf0ba8a5f12d563bb4c6f0f26a
>
> Diff: https://reviews.apache.org/r/31127/diff/
>
>
> Testing
> -------
>
> I've run the packaging target and the shell/lib directory size decreased from 46MB to 28MB (almost a half!), excluding all the testing dependencies.
>
>
> Thanks,
>
> Jarek Cecho
>
>
Re: Review Request 31127: SQOOP-2109 Sqoop2: Shell module is
including test dependencies when building binary artifact
Posted by Jarek Cecho <ja...@apache.org>.
> On Feb. 17, 2015, 7:52 p.m., Veena Basavaraj wrote:
> > pom.xml, line 475
> > <https://reviews.apache.org/r/31127/diff/1/?file=866485#file866485line475>
> >
> > does not this need a license update? if so please indicate that and not sure if we need a new RB to update LICENSE.txt
Ideally we should update LICENSE.txt file for every jar that we are shipping. Currently this is not done for several of them. We should do it, but it doesn't seem blocker for 1.99.5 and hence I did not do this exercise as part of this patch.
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31127/#review72769
-----------------------------------------------------------
On Feb. 17, 2015, 7:57 p.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31127/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2015, 7:57 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2109
> https://issues.apache.org/jira/browse/SQOOP-2109
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> There has been several things that I had to fix in order to get this right:
>
> * Shell packaging was pulling in all dependencies regardless of their scope, I had to manually switch it from "test" to "runtime"
> * Common was directly depending on common-test without properly specifying testing scope
> * Security module was completely missing dependency on com.bueust (that wasn't problem because 2) has pulled it in transitively)
>
>
> Diffs
> -----
>
> common/pom.xml 9e593b2
> pom.xml d48389e
> security/pom.xml e4e61cc
> shell/pom.xml e48c86a
>
> Diff: https://reviews.apache.org/r/31127/diff/
>
>
> Testing
> -------
>
> I've run the packaging target and the shell/lib directory size decreased from 46MB to 28MB (almost a half!), excluding all the testing dependencies.
>
>
> Thanks,
>
> Jarek Cecho
>
>
Re: Review Request 31127: SQOOP-2109 Sqoop2: Shell module is
including test dependencies when building binary artifact
Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31127/#review72769
-----------------------------------------------------------
pom.xml
<https://reviews.apache.org/r/31127/#comment118841>
does not this need a license update? if so please indicate that and not sure if we need a new RB to update LICENSE.txt
- Veena Basavaraj
On Feb. 17, 2015, 11:17 a.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31127/
> -----------------------------------------------------------
>
> (Updated Feb. 17, 2015, 11:17 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-2109
> https://issues.apache.org/jira/browse/SQOOP-2109
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> There has been several things that I had to fix in order to get this right:
>
> * Shell packaging was pulling in all dependencies regardless of their scope, I had to manually switch it from "test" to "runtime"
> * Common was directly depending on common-test without properly specifying testing scope
> * Security module was completely missing dependency on com.bueust (that wasn't problem because 2) has pulled it in transitively)
>
>
> Diffs
> -----
>
> common/pom.xml 9e593b2e23d54db34ad5737c08637365792e6537
> pom.xml d48389ee60c1fbc5fa76e85a577d48f1e2032aec
> security/pom.xml e4e61ccaa43aa62c7db66fe72ade3bc25b4155aa
> shell/pom.xml e48c86a20f0bb5bf0ba8a5f12d563bb4c6f0f26a
>
> Diff: https://reviews.apache.org/r/31127/diff/
>
>
> Testing
> -------
>
> I've run the packaging target and the shell/lib directory size decreased from 46MB to 28MB (almost a half!), excluding all the testing dependencies.
>
>
> Thanks,
>
> Jarek Cecho
>
>