Add support for Java 25 and Spring Boot 4#881
Conversation
paveljandejsek
left a comment
There was a problem hiding this comment.
Nice job, thanks a lot!
I have mainly just questions and a couple of nitpicks 🙂
Main points for me:
- What do we want the next version to be? 2.1.0 vs 3.0.0?
- What was the reason for Java 17 support being removed?
- The whole datasource stuff in the test app feels weird, I would hope autoconfig and liquibase to handle the heavy lifting instead of doing it manually 🙂
- We should probably take a look at jbehave runner as well in some future PR (since Boot 4 brings in JUnit 6 which the runner currently does not support)
|
|
||
| <!-- enable annotation processing for lombok in java 23: https://github.com/projectlombok/lombok/issues/3752 --> | ||
| <maven.compiler.proc>full</maven.compiler.proc> | ||
| <maven.compiler.source>${java.version}</maven.compiler.source> |
There was a problem hiding this comment.
Do we really need to set source and target explicitly? Isn't the <maven.compiler.release>${java.version}</maven.compiler.release> set by Boot parent enough for our cases?
There was a problem hiding this comment.
It is necessary to set atleast target due to gmavenplus, otherwise the tests won't start. But source can be deleted so it is removed in the next commit.
There was a problem hiding this comment.
And here I was blissfully forgetting about the joys of mixing groovy and java code in one codebase 😄
Thanks for the explanation and investagation, could you please add a comment above it similar to the Lombok one on the compiler.proc above it? Just so in the future we know that its there because of gmavenplus
| @@ -0,0 +1,13 @@ | |||
| CREATE TABLE IF NOT EXISTS PERSON ( | |||
There was a problem hiding this comment.
Shouldn't this be handled by the already existing changelog instead?
There was a problem hiding this comment.
If I remember correctly, tests were failing for not knowing table Person even though it is mentioned in the changelog.
There was a problem hiding this comment.
I think the issue might be with the change of starters, see:
https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-4.0-Migration-Guide
If I add the spring-boot-starter-liquibase dependency (liquibase-core is no longer needed) I can see the migrations running, so the init script should not be needed (at least I hope so)🤞
| dataSourceBuilder.username(env.getProperty("db.username")); | ||
| dataSourceBuilder.password(env.getProperty("db.password")); | ||
| return dataSourceBuilder.build(); | ||
| DriverManagerDataSource ds = new DriverManagerDataSource(); |
There was a problem hiding this comment.
May I ask why the replacement? I thought the datasource builder still exists? Or is it not usable for us anymore due to some reason?
There was a problem hiding this comment.
If I am not mistaken there has been changes how it works in Spring Boot 4 and it causes issues H2 databases in tests.
There was a problem hiding this comment.
Ok so in the end I was able to make it work with the previous code, the issue was the same as with the liquibase - i.e. instead of spring-jdbc dependency in jbehave-support-core pom we now need the starter one:
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-jdbc</artifactId>
</dependency>
https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-4.0-Migration-Guide
| private HttpEntity createRequestEntity(ExamplesTable data) throws IOException { | ||
| if (data == null) { //return dummy | ||
| return new HttpEntity<>("", null); | ||
| return new HttpEntity<>("", HttpHeaders.EMPTY); |
There was a problem hiding this comment.
Just curious - did the old null cause issues? But yeah, this new version is definitely more readable 👍
There was a problem hiding this comment.
The previous implementation is not usable anymore due to the latest changes and Intellj Idea has been screaming on me 🙂
| basic [reporting](jbehave-support-core/docs/Reporting.md). | ||
|
|
||
| Currently supported Java versions are 17, 21 and 23 (latest LTS and latest version). | ||
| Currently supported Java versions are 21 and 25 (latest LTS and latest version). |
There was a problem hiding this comment.
Can I ask what is the reason for dropping Java 17 support? As far as I know Spring Boot 4 and Spring 7 should still work with it? Or I am wrong? (haven't tried myself, just asking)
There was a problem hiding this comment.
Some of the changes were not compatible with Java 17 and since Java 21 is used for quite some time, I though it is not necessary to make new version of JBehave compatible with Java 17. But if you think we should still keep compatibility with Java 17, I can make it happen 🙂
There was a problem hiding this comment.
I would prefer compability with Java 17 as it is still supported LTS verions of Java :)
| public class SecurityConfig { | ||
|
|
||
| @Bean | ||
| PathPatternRequestMatcherBuilderFactoryBean requestMatcherBuilder() { |
There was a problem hiding this comment.
Can I ask why is this needed? I was under the impression the requestMatchers("xxx") work by default? Or was that changed somehow?
There was a problem hiding this comment.
After upgrade I was dealing with some errors since antMatchers has been deleted and after some googling, I have found this solution.
There was a problem hiding this comment.
I had some issues with fully running the pipeline locally (my docker env is fucked atm), but when running the app as a standalone I was able to get it to run without this bean and with the remaining security config as is... Can you please try as well? Or let me know what the issue was/is so I could try it on my side as well?
There was a problem hiding this comment.
Ok so the docker being borked was actually caused by testcontainers - do you think you could remove the version.testcontainers and the testcontainers dependency (line 202) from the main pom? Boot 4 now seems to include testcontainers bom (and a much newer version than we had previously), so we can rely on the version from the spring boot now.... (Or if you want I can do it in my own commit either to your branch/this PR, or I can do it after we merge this in a separate PR, up to you)
Thank you for your review ! 🙂 I tried my best and surely you can see that I am not on your java knowledge level. I hope that I have answered everything. As I wrote, I would prefer to use version 3.0.0 since there is update of spring boot and java. I definitely agree that there should come some other PR to update other things. And Compatibility with Java 21 ? I can make it happen, I just though that it won't be necessary and compatibility with Java 21 will be enough but it is your call. It would take just couple of small changes to make it compatible still with Java 17 |
No need to be modest, you probably code more than I do lately, as you can see half of the stuff from me were questions how the stuff even works nowadays 😄 I have put a couple more replies to the comments, I still want to go over the H2 stuff a bit more on my side (and fix my env to be able to run clean verify without errors locally, but that's my issue) - hopefully tomorrow evening I should have the rest of the replies for you |
|
Sorry for being late, the docker issue for me took longer than it should have (OS upgrade, docker updates, fiddling around with some random stuff before I thought about checking testcontainers themselves 🤦♂️), I put the remaining comments in there... Thank you again for the nice work! |
Thank you for the latest comments. I hope that I have changed everything, that was mentioned 😄 I have kept java 17 in the project and made it compatible so I think, that version 2.1.0 would be valid |
paveljandejsek
left a comment
There was a problem hiding this comment.
Apologies, but I found a couple more nitpicks (tried to put suggestions where I could), mostly probably just forgotten stuff from the editing I think... But otherwise I think we are good to go, once you manage to solve them I will try to get the new version 2.1.0 out once we have it merged 🙂
Please don't hate me 😄
| path: jbehave-support-core/target/reports | ||
| - name: Upload test coverage for integration tests | ||
| if: matrix.java == 17 | ||
| if: matrix.java == 25 |
There was a problem hiding this comment.
| if: matrix.java == 25 | |
| if: matrix.java == 17 |
We need same version for sonar as we are uploading the artifacts for (just a rollback of probably forgotten changes)
| restore-keys: | | ||
| ${{ runner.os }}-maven- | ||
| - name: Set up Java 17 | ||
| - name: Set up Java 21 |
There was a problem hiding this comment.
| - name: Set up Java 21 | |
| - name: Set up Java 17 |
We need same version for sonar as we are uploading the artifacts for (just a rollback of probably forgotten changes)
| uses: actions/setup-java@v4 | ||
| with: | ||
| java-version: 17 | ||
| java-version: 21 |
There was a problem hiding this comment.
| java-version: 21 | |
| java-version: 17 |
We need same version for sonar as we are uploading the artifacts for (just a rollback of probably forgotten changes)
| <tags>docker.io/library/jbehave-support-core-test-app:latest</tags> | ||
| <env> | ||
| <BP_JVM_VERSION>17</BP_JVM_VERSION> | ||
| <BP_JVM_VERSION>21</BP_JVM_VERSION> |
There was a problem hiding this comment.
| <BP_JVM_VERSION>21</BP_JVM_VERSION> | |
| <BP_JVM_VERSION>17</BP_JVM_VERSION> |
We need the lowest version here for pipeline on 17 to work I think? (basically just a rollback of probably forgotten changes?)
|
|
||
| > #### Java 17 | ||
| >We now currently require Java 17 as a minimal Java version (previously 8). | ||
| > #### Java 25 |
There was a problem hiding this comment.
| > #### Java 25 | |
| > #### Java 17 |
| import org.springframework.boot.SpringBootConfiguration; | ||
| import org.springframework.boot.autoconfigure.EnableAutoConfiguration; | ||
| import org.springframework.boot.jdbc.DataSourceBuilder; | ||
| import org.springframework.jdbc.datasource.DriverManagerDataSource; |
There was a problem hiding this comment.
I think a couple of forgotten unused imports?
|
|
||
| db: | ||
| url: jdbc:h2:tcp://localhost:11112/mem:test;MODE=ORACLE | ||
| url: jdbc:h2:mem:test;MODE=ORACLE;DB_CLOSE_DELAY=-1 |
There was a problem hiding this comment.
| url: jdbc:h2:mem:test;MODE=ORACLE;DB_CLOSE_DELAY=-1 | |
| url: jdbc:h2:tcp://localhost:11112/mem:test;MODE=ORACLE |
I think we now need the previous version back? Since the TestAppContainer uses fixed port...
No need for hate. Those were my mistakes that I have forgot to return back settings while I was trying to run those tests locally. 😄 Sorry for that. I think that everything should be (hopefully) correct for now. And somehow optimalization of unused imports was not marked (even though I was using it. Weird 😄 ) |
|
Looking good, thank you very much! Approved from my side, I will try to get it merged and released once I am able. |
Add support for java 25 and spring boot 4