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 code generation for Hazelcast cache provider #387

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

vcupelloni
Copy link

Hi, I implemented this code because of the issue 383

I manually tested the generated code for both a monolith application and a microservice (using both Consul and JHRegistry).

I have no experience in writing tests in this area, if there are existing ones to study (I couldn't find them) or there is guidance, let me know so I can integrate the code.

Please let me know if it helps

@CLAassistant
Copy link

CLAassistant commented Aug 24, 2024

CLA assistant check
All committers have signed the CLA.

@DanielFran
Copy link
Member

@vcupelloni You need to sign the CLA

@vcupelloni
Copy link
Author

@vcupelloni You need to sign the CLA

I thought I did, now it should be ok.
image

@mshima
Copy link
Member

mshima commented Aug 26, 2024

@vcupelloni the git commit is probably created with different email.

@mraible mraible requested a review from mshima September 10, 2024 13:55
@@ -230,10 +230,10 @@ dependencies {
implementation "com.hazelcast:hazelcast"
<%_ } _%>
<%_ if (cacheProvider === 'hazelcast' && enableHibernateCache) { _%>
implementation "com.hazelcast:hazelcast-hibernate53"
implementation "com.hazelcast:hazelcast-hibernate53:5.2.0"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
implementation "com.hazelcast:hazelcast-hibernate53:5.2.0"
implementation "com.hazelcast:hazelcast-hibernate53:<%- javaDependencies['hazelcast-hibernate53'] %>"

And add this dependency to https://github.com/jhipster/generator-jhipster-micronaut/blob/main/generators/micronaut/resources/gradle/libs.versions.toml
So it will be updated automatically.

Copy link
Member

Choose a reason for hiding this comment

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

Maven counterpart should be added too.
Or inject dependencies using source.addJavaDependencies here:

addDependencies({ application, source }) {

source.addJavaDependencies will add to gradle and maven.

Copy link
Author

Choose a reason for hiding this comment

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

@mshima I worked on source.addJavaDependencies and implemented dependency injection for gradle taking a cue from what had already been done for the liquibase section generator.

However, I only handled the Hazelcast dependencies leaving the other cache providers as they were in the build.gradle.ejs to avoid regressions and not knowing the impacts.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

This file is the published micronaut-platform bom, we have it embedded so we can a dependency version matching it.
It should be not changed.

Copy link
Member

@mshima mshima left a comment

Choose a reason for hiding this comment

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

We have a common api for maven/gradle using addJavaDependencies/addJavaDefinition.

Comment on lines +239 to +240

version: '${hazelcast_version}',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
version: '${hazelcast_version}',
version: javaDependencies['hazelcast_version'],

Comment on lines +246 to +247

version: '${micronaut_cache_version}',
Copy link
Member

Choose a reason for hiding this comment

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

Inherited by the micronaut-platform bom.
Don't need to add a version.

Suggested change
version: '${micronaut_cache_version}',

Comment on lines +42 to +48
<%_ if (cacheProvider === 'hazelcast') { _%>
hazelcast_version=<%= javaManagedProperties['hazelcast.version'] %>
micronaut_cache_version=<%= javaManagedProperties['micronaut.cache.version'] %>
<%_ if (enableHibernateCache) { _%>
hazelcast_hibernate_version=<%= javaManagedProperties['hazelcast-hibernate.version'] %>
<%_ } _%>
<%_ } _%>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<%_ if (cacheProvider === 'hazelcast') { _%>
hazelcast_version=<%= javaManagedProperties['hazelcast.version'] %>
micronaut_cache_version=<%= javaManagedProperties['micronaut.cache.version'] %>
<%_ if (enableHibernateCache) { _%>
hazelcast_hibernate_version=<%= javaManagedProperties['hazelcast-hibernate.version'] %>
<%_ } _%>
<%_ } _%>

Comment on lines +77 to +85
}else if(application.buildToolGradle){
const definition = getCacheProviderGradleDefinition(application.cacheProvider, application.javaDependencies);

if(definition){
source.addGradleDependencies?.(definition.base.dependencies);
if (application.enableHibernateCache && definition.hibernateCache) {
source.addGradleDependencies?.(definition.hibernateCache.dependencies);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}else if(application.buildToolGradle){
const definition = getCacheProviderGradleDefinition(application.cacheProvider, application.javaDependencies);
if(definition){
source.addGradleDependencies?.(definition.base.dependencies);
if (application.enableHibernateCache && definition.hibernateCache) {
source.addGradleDependencies?.(definition.hibernateCache.dependencies);
}
}

Comment on lines +69 to +70

if(application.buildToolMaven){
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(application.buildToolMaven){
const { javaDependencies, enableHibernateCache } = application;
if (application.cacheProviderHazelcast) {
source.addJavaDefinition(
{
versions: [{ name: 'hazelcast', version: javaDependencies.hazelcast }],
dependencies: [
javaxCacheApi,
{ groupId: 'com.hazelcast', artifactId: 'hazelcast', versionRef: 'hazelcast' },
{ groupId: 'io.micronaut.cache', artifactId: 'micronaut-cache-hazelcast' },
],
},
{
condition: enableHibernateCache,
dependencies: [{ groupId: 'com.hazelcast', artifactId: 'hazelcast-hibernate53', versionRef: 'hazelcast' }],
},
);
} else if (application.buildToolMaven){

@mraible
Copy link
Contributor

mraible commented Oct 31, 2024

@vcupelloni Are you still interested in this PR? There's quite a few suggestions that need to be committed.

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

Successfully merging this pull request may close these issues.

6 participants