search

wildfly-extras / wildfly-jar-maven-plugin

WildFly Bootable JAR
https://docs.wildfly.org/bootablejar/
Apache License 2.0
56 stars 40 forks source link

Add jpms properties for jdk17. #313

Closed jedla97 closed 1 year ago

jedla97 commented 2 years ago

Hi, this PR adding wildfly jpms properties for testing with jdk17.

jfdenise commented 2 years ago

@jedla97 we have bootable JAR tests that run on JDK17 already, you can check github actions. I am not sure which issue your PR is addressing? Thank-you.

jedla97 commented 2 years ago

Hi @jfdenise , it's cover the qe joint efforts to make sure that jpms properties (for server as well for client side) is propagated correctly. And as QE we need to have consistent JPMS parameters in our TSs so new product (server/client) updates (e.g. new usage of JVM private packages, that was not used earlier in TS use-cases) can’t break our TS behavior.

jamezp commented 2 years ago

I really think we need a wider discussion on this. We're not really testing anything other than these properties work. It really could hide actual issues as ideally we are able to run on JDK 17 with no extra JVM configurations.

jfdenise commented 2 years ago

@jamezp , I agree. BTW, the changes have no impact on the tests. I have been discussing with @jedla97 to challenge the PR. We will see how it goes, but on hold for now.

marekkopecky commented 2 years ago

as ideally we are able to run on JDK 17 with no extra JVM configurations.

This is not a true according to the analysis provided by @bstansberry . Maybe that developers should really discuss it first themself, but now the test development of JDK17 is in progress based on the current analysis.

jedla97 commented 1 year ago

Hi @jfdenise as we talk I change the MR. I talk with qe coordinator for jdk17 and come to the agreement that for this it's not necessary to test the server side jpms properties, but we want still test at least the client side.

jfdenise commented 1 year ago

@jedla97 , I am not sure why you need them on the client side? These are for the server right? I just ran the tests to see if we have any impact.

marekkopecky commented 1 year ago

I am not sure why you need them on the client side?

@jfdenise Generally we should use the same client-side parameters in our tests like the parameters that users should use in production.

jfdenise commented 1 year ago

@jedla97 , merged thank-you.