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

fix: java 17 for spring samples and maven archetypes #1156

Merged
merged 13 commits into from
Oct 7, 2022

Conversation

aludwiko
Copy link
Contributor

@aludwiko aludwiko commented Oct 6, 2022

References #xxxx

@lightbend-cla-validator
Copy link
Collaborator

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.
Please review the new CLA and sign it before we proceed with reviewing this pull request:

https://www.lightbend.com/contribute/cla

@github-actions github-actions bot added Documentation Improvements or additions to the documentation kalix-runtime Runtime and SDKs sub-team labels Oct 6, 2022
@aludwiko aludwiko changed the title [WIP] testing java 17 for samples [WIP] java 17 for samples Oct 6, 2022
Copy link
Member

@octonato octonato left a 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? 🤷

@aludwiko aludwiko force-pushed the 7608-java-17-for-samples-and-archetypes branch from 1502d61 to 5578d1f Compare October 6, 2022 15:05
@aludwiko aludwiko changed the title [WIP] java 17 for samples fix: java 17 for spring samples and maven archetypes Oct 7, 2022
Copy link
Contributor

@johanandren johanandren left a 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).

@aludwiko
Copy link
Contributor Author

aludwiko commented Oct 7, 2022

sans the hardcoded versions and the docker-hub account placeholders.

yeah, my bad, fixed.

@aludwiko aludwiko force-pushed the 7608-java-17-for-samples-and-archetypes branch from b02c5d8 to 6811401 Compare October 7, 2022 12:36
@octonato
Copy link
Member

octonato commented Oct 7, 2022

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).

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,
https://docs.kalix.io/java/quickstart-template.html. The new archetype is using 17. Maybe we should revert it for now. And only concentrate on the Spring one.

If we move the java (gRPC) archetype, than we can also move all the samples all together.

@octonato
Copy link
Member

octonato commented Oct 7, 2022

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.

@octonato
Copy link
Member

octonato commented Oct 7, 2022

@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.

@octonato
Copy link
Member

octonato commented Oct 7, 2022

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

@aludwiko
Copy link
Contributor Author

aludwiko commented Oct 7, 2022

@octonato grpc archetypes reverted

Copy link
Member

@octonato octonato left a 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

@octonato octonato merged commit 0c5d561 into main Oct 7, 2022
@octonato octonato deleted the 7608-java-17-for-samples-and-archetypes branch October 7, 2022 18:31
@beritou
Copy link
Contributor

beritou commented Oct 7, 2022

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?

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"? 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Improvements or additions to the documentation java-sdk-protobuf kalix-runtime Runtime and SDKs sub-team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants