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

feat(integrations/spring): add spring project module #4988

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

shoothzj
Copy link
Member

@shoothzj shoothzj commented Aug 9, 2024

Rationale for this change

Related to #4756
See more context in https://lists.apache.org/thread/13fhp5s2lrbpgdl129gt7zbpd3gms8bb

What changes are included in this PR?

  • introduce spring opendal modules

@Xuanwo
Copy link
Member

Xuanwo commented Aug 9, 2024

Thanks a lot for the PR. I'll get to this after OpenDAL 0.49 is released.

@shoothzj
Copy link
Member Author

ping @Xuanwo @tisonkun

@@ -0,0 +1 @@
# Spring OpenDAL
Copy link
Member

Choose a reason for hiding this comment

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

Please refer to the other README files to add some basic information about this integration.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to call it "Apache OpenDAL Spring Integrations". Or in details like "Apache OpenDAL Spring Boot Starter".

This follow the name conventions that a thrid party plugin name as <third-party>-spring-xxx.

<modelVersion>4.0.0</modelVersion>

<groupId>org.apache.opendal</groupId>
<artifactId>spring-opendal-parent</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

I take a look over spring-boot-starter-data-redis and found that they are in three artifactId:

  • spring-data-redis
  • spring-boot-starter-data-redis
  • spring-boot-starter-data-redis-reactive

Should we follow the same pattern that have three artifact instead of module?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Xuanwo I think you may misunderstand module and artifactId's definition. Every library has an artifactId, which is the smallest unit of reference. Some libraries are responsible for managing other libraries and do not directly contain code themselves; they use a to manage modules or sub-projects (in Maven 4.0, these will be referred to as sub-projects instead of modules).

<version>0.0.1-SNAPSHOT</version>
<packaging>pom</packaging>
<modules>
<module>spring-opendal</module>
Copy link
Member

Choose a reason for hiding this comment

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

How about spring-data-opendal? I'm guessing it's different with spring-opendal?

<packaging>pom</packaging>
<modules>
<module>spring-opendal</module>
<module>spring-starter-opendal</module>
Copy link
Member

Choose a reason for hiding this comment

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

How about spring-boot-starter-data-opendal?

<modules>
<module>spring-opendal</module>
<module>spring-starter-opendal</module>
<module>spring-starter-opendal-reactive</module>
Copy link
Member

Choose a reason for hiding this comment

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

How about spring-boot-starter-data-opendal-reactive?

</dependency>
<dependency>
<groupId>org.apache.opendal</groupId>
<artifactId>opendal-java</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

should be opendal

Copy link
Member Author

Choose a reason for hiding this comment

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

@Xuanwo Fixed

@shoothzj
Copy link
Member Author

@Xuanwo @tisonkun

I hope that in the end we use the following naming convention:

<module>opendal-spring</module>
<module>opendal-spring-starter</module>
<module>opendal-spring-starter-reactive</module>

Reasons:

  1. Indeed, opendal-spring-starter would be more suitable for third parties. spring-starter-opendal might lead users to expect version management aligned with Spring.
  2. As for opendal-spring-starter or opendal-spring-data-starter, I believe the data part is unnecessary. Spring Data is a sub-project of Spring, and we currently do not have that kind of relationship.

Let me know if there's anything wrong.

@shoothzj
Copy link
Member Author

@PsiACE @tisonkun @Xuanwo PTAL,thanks

Signed-off-by: ZhangJian He <shoothzj@gmail.com>
@Xuanwo
Copy link
Member

Xuanwo commented Aug 14, 2024

2. As for opendal-spring-starter or opendal-spring-data-starter, I believe the data part is unnecessary. Spring Data is a sub-project of Spring, and we currently do not have that kind of relationship.

Hi, it looks good to me to remove data, but boot should still be kept? My current understanding is there is no spring-starter but spring-boot-starter.

@tisonkun
Copy link
Member

Yes. Let me push a follow up and merge this.

Signed-off-by: tison <wander4096@gmail.com>
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment on lines +37 to +41
<parent>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-dependencies</artifactId>
<version>${spring-boot.version}</version>
</parent>
Copy link
Member

Choose a reason for hiding this comment

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

Personally I'd prefer to use dependency management rather than inherit from spring boot, as in https://docs.spring.io/spring-boot/maven-plugin/using.html#using.import

But this is OK for now.

@tisonkun tisonkun merged commit 24b709e into apache:main Aug 14, 2024
27 checks passed
@shoothzj shoothzj deleted the spring-integeration-module branch August 14, 2024 06:07
@shoothzj
Copy link
Member Author

@Xuanwo Sorry, It was a typo, boot is in the code indeedly.

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

Successfully merging this pull request may close these issues.

4 participants