-
Notifications
You must be signed in to change notification settings - Fork 579
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
Use jandex index in MP quickstart, update jandex-plugin version #714
Conversation
<plugin> | ||
<groupId>org.jboss.jandex</groupId> | ||
<artifactId>jandex-maven-plugin</artifactId> | ||
<version>1.0.5</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to (or need to) use Jandex 2.1.1 (@tjquinno?) then this must be version 1.0.6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. We moved to 2.1.1 in the workspace. I'll update these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our build (top-level project pom.xml) specifies Jandex 2.1.1.Final. Laird, do you have a reference for the 1.0.6 requirement for 2.1.1.Final?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep; see https://github.com/wildfly/jandex-maven-plugin/commits/master and read up from April 3, 2019.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we specified 2.1.1.FInal already in our top-level pom's dep. mgt. section does that influence the plug-ins' dependencies too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. If you wanted to effectively use 1.0.5 for some reason with Jandex 2.1.1, thus effectively creating a very clean Frankenstein jandex-maven-plugin that is exactly equivalent to 1.0.6 as far as I can tell, you would add a <dependencies>
stanza to the relevant <plugin>
element in <pluginManagement>
that referenced Jandex 2.1.1. I think it's simpler just to change the jandex-maven-plugin version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yes. I wasn't suggesting we rely on that (even if it did work). I was just curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the jandex plugin version in the top level pom.xml, and the jandex versions in the quickstart MP pom.xml. I've also updated the bookstore test app to use the jandex plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with the one note and pending a possible change in the plug-in version to 1.0.6 per Laird's note.
<goals> | ||
<goal>jandex</goal> | ||
</goals> | ||
<phase>process-classes</phase> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the default phase for the plug-in. If you want to keep it here for clarity, fine. If you want to remove it for minimalism, fine.
No description provided.