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

Add the ability for extensions to exclude GraalVM config from jars #19649

Merged
merged 1 commit into from
Aug 26, 2021

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Aug 25, 2021

Resolves: #17482

@jaikiran
Copy link
Member

At a high level, keeping aside the 2 arg value comment that I added, I think we should perhaps make this ExcludeConfigBuildItem a bit more usable for extension developers, if possible. What I mean is, the value they provide to this build item is expected to be a jar file name (or regex). Expecting them to hardcode/pass these jar file names or regex I think could be brittle (because they then have to go check how the dependency jar name looks like, does it have a version included in its name etc...). Perhaps we should allow them to pass a Class instance to this build item and we internally then find the java.security.CodeSource of that specific class and then apply the --exclude-config against that jar name (which we get dynamically via the CodeSource)? For example (a completely random one), if they want to exclude a MySQL driver jar's native configuration resource, then instead of knowing that jar's name, they could just specify the Class instance of the MySQL driver (or maybe even the classname?) and we then go find the right jar that needs to be excluded. I think expecting a Class (name) or some such thing in the build item will probably make this much more robust and easy to use than jar names.

@geoand geoand marked this pull request as ready for review August 25, 2021 11:35
@geoand
Copy link
Contributor Author

geoand commented Aug 25, 2021

@jaikiran yeah that makes sense, but I would first like @Sanne and @ppalaga to give it a try with the use cases they have in mind.
If it indeed turns out to be burden to specify strings, we can certainly make it easier using things like you mention.

Since we are only at the beginning of the 2.3 development cycle, we have plenty of time to improve on this.

@geoand geoand requested review from jaikiran and Sanne August 25, 2021 11:41
Copy link
Member

@Sanne Sanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in principle, but there's some changes that seem unrelated.

Copy link
Contributor

@ppalaga ppalaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks good, thanks. I do not think we have anything waiting urgently for this ATM.

@geoand
Copy link
Contributor Author

geoand commented Aug 26, 2021

@Sanne are you OK with this?

Copy link
Member

@Sanne Sanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geoand yes sure. I was hoping to try it out as well, but couldn't get to that yet - I'll do it soon but it's no reason to hold you back.

@Sanne Sanne merged commit 164f632 into quarkusio:main Aug 26, 2021
@quarkus-bot quarkus-bot bot added this to the 2.3 - main milestone Aug 26, 2021
@geoand geoand deleted the #17482 branch August 26, 2021 12:28
@geoand
Copy link
Contributor Author

geoand commented Aug 26, 2021

👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass --exclude-config programatically
4 participants