-
Notifications
You must be signed in to change notification settings - Fork 39
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
fix: java 17 for spring samples and maven archetypes #1156
Conversation
Hi @aludwiko, Thank you for your contribution! We really value the time you've taken to put this together. We see that you have signed the Lightbend Contributors License Agreement before, however, the CLA has changed since you last signed it. |
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.
Looks much better with Java 17.
I wonder why they decided to make the access to the fields look like a method call, eg: name()
. Why a direct access to a public final method isn't good?
Maybe we are allowed to overwrite those methods? 🤷
samples/spring-eventsourced-customer-registry/src/main/java/customer/api/CustomerEvent.java
Show resolved
Hide resolved
1502d61
to
5578d1f
Compare
Co-authored-by: Renato Cavalcanti <[email protected]>
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 sans the hardcoded versions and the docker-hub account placeholders.
Note that we will need to bump the Java QuickStart doc pages as well once this is released as they now say JDK 11 is or later is the requirement but after this the quickstarts require 17:
https://docs.kalix.io/java/quickstart/cr-value-entity-java.html#_before_you_begin
I wonder if we should bump the Scala samples as well (separate PR), not that it changes anything for the Scala code but to get the same base JVM image for all JVM SDK services (and potential runtime improvements GC etc).
samples/spring-eventsourced-counter/src/main/java/com/example/CounterEvent.java
Show resolved
Hide resolved
samples/spring-eventsourced-customer-registry/src/main/java/customer/view/CustomerView.java
Show resolved
Hide resolved
yeah, my bad, fixed. |
b02c5d8
to
6811401
Compare
The quickstart for the customer register is still using jdk 11. This PR is only changing the Spring samples. However, we should change the quickstart with template (archetype) page, If we move the java (gRPC) archetype, than we can also move all the samples all together. |
To make my point, using Java 17 is interesting for the Spring SDK because users can use records and sealed interfaces. That's less relevant for the gRPC one because we don't control the Java classes. |
@aludwiko, can we revert the grpc archetypes and do it in another PR? Then we can update all the Java gRPC samples. And than another PR for the Scala ones. |
Ok, to update the docs, we need to change a variable in the Antora Makefile. That will change the JDK version for all pages that mention it. So, either we change it for all samples or we only do it for the Spring ones now. Since we don't have Spring docs yet, I think it's fine |
@octonato grpc archetypes reverted |
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.
Great! Waiting for green build to merge
I also don't like the look of how fields are accessed. Maybe this was done to emphasize the fact that these are "getter methods"? 🤷 |
References #xxxx