Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid dependencyExclusion doesn't fail the build #147

Closed
rnc opened this issue Jul 17, 2015 · 3 comments
Closed

Invalid dependencyExclusion doesn't fail the build #147

rnc opened this issue Jul 17, 2015 · 3 comments
Assignees
Labels
Milestone

Comments

@rnc
Copy link
Contributor

rnc commented Jul 17, 2015

If we have an invalid dependencyExclusion e.g. change

-dependencyExclusion.junit\:*@*=
+dependencyExclusion.junit@*=

in src/it/depex-multiple-wildcard/test.properties then the exception thrown ( org.commonjava.maven.atlas.ident.ref.InvalidRefException ) is a RuntimeException and is not handled by org.commonjava.maven.ext.manip.ManipulationManager or org.commonjava.maven.ext.manip.ManipulatingEventSpy

@rnc rnc added the bug label Jul 17, 2015
@jdcasey jdcasey modified the milestones: 1.6, someday Jul 21, 2015
@rnc rnc modified the milestones: 1.6, someday Jul 24, 2015
@rnc
Copy link
Contributor Author

rnc commented Jul 29, 2015

Turns out this is due to an InvalidRefException being thrown from ProjectRef.parse within DependencyManipulator. This leads onto a number of points:

  • We should ensure RuntimeExceptions thrown during PME do fail the build.
  • @jdcasey : should InvalidRefException be a RuntimeException or a normal Exception?
  • In a number of the Manipulators we catch the InvalidRefException, log a warning and continue. I think that we should fail the build in the case of an invalid configuration. There is some benefit to catching the InvalidRef and rethrowing in that useful contextual error logging can be done at that point.
  • And in Project we are throwing IllegalStateException instead of ManipulationException

@jdcasey
Copy link
Member

jdcasey commented Jul 29, 2015

@rnc it should probably be a checked exception. I have a feeling this will have HUGE implications in terms of required code changes, though. It's probably something we should do, though.

@rnc rnc assigned rnc and unassigned zzoubkova Jul 29, 2015
@rnc
Copy link
Contributor Author

rnc commented Jul 29, 2015

Added Commonjava/atlas#31

rnc added a commit to rnc/pom-manipulation-ext that referenced this issue Jul 29, 2015
jdcasey added a commit that referenced this issue Jul 29, 2015
Issue #147 : Ensure runtime exceptions fail the build. Better logging.
@jdcasey jdcasey closed this as completed Jul 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants