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

General Feedback about Service Connections #34760

Closed
eddumelendez opened this issue Mar 25, 2023 · 7 comments
Closed

General Feedback about Service Connections #34760

eddumelendez opened this issue Mar 25, 2023 · 7 comments
Labels
status: superseded An issue that has been superseded by another theme: containers Testcontainers and Docker Compose features

Comments

@eddumelendez
Copy link
Contributor

First of all, thanks for introducing this feature. I already started using it in my examples

Also, sorry for writing all the way in this issue, but I think it would be much better to understand how to proceed instead of opening many issues/PRs

  1. 🐛 bug? The only examples I couldn't migrate were related to Yugabyte because of the driver. It's missing here

  2. 🆕 I would like to ask if you will introduce RedpandaServiceConnection. Redpanda offers a Kafka-compatible API, and it is very fast. I have almost everything to submit a PR, but I want to ask first :)

  3. 🔜 🤞🏽 When the time comes, I think adding support for Pulsar would be great. I just created an issue in their repo in case the autoconfiguration support in spring-boot takes longer Add PulsarServiceConnection support when Spring Boot 3.2.0 is supported spring-pulsar#384

  4. ❓ What's the vision of ServiceConnections beyond spring-boot? I mean support in spring-cloud. I think this can be used in spring-cloud-consul, spring-cloud-vault, and spring-cloud-zookeeper. I ask because the Config Data approach offered by those projects could benefit from it. Currently, system properties needs to be added or registered through @ContextConfiguration(initializers = ...) which with ServiceConnections could possible be fixed too.

  5. ❓ Is RedisServiceConnection from org.springframework.boot.test.autoconfigure.data.redis instead of org.springframework.boot.test.autoconfigure.redis?. Asking for consistency reasons

  6. ❓ Given the current state of the art, can we introduce HazelcastServiceConnection expecting a GenericContainer similar to Redis? Given the approach to introducing those new integrations, the project should be ready to accept them if needed.

If 1, 2, 5, and 6 make sense to you, I can probably help with some or pair with someone else who wants to contribute. I guess a couple of those issues will be fixed by the team 😀

Once again, thank you so much for adding this feature.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 25, 2023
@quaff
Copy link
Contributor

quaff commented Mar 27, 2023

  1. 🐛 bug? The only examples I couldn't migrate were related to Yugabyte because of the driver. It's missing here

Why would you configure jdbc driver manually? It's the responsibility of java.sql.DriverManager to load proper driver for specific jdbc url.

@wilkinsona
Copy link
Member

wilkinsona commented Mar 27, 2023

This is great feedback. Thanks very much, @eddumelendez.

  1. Yes, this is a bug. Thanks for catching it. I have opened DataSource auto-configuration does not use the driver class name from JdbcConnectionDetails #34777
  2. We can certainly consider it. It feels like it's within scope given that Spring Boot supports Kafka and Testcontainers, and Testcontainers has a Redpanda module. If you have the code pretty much ready, please feel free to submit a PR. Alternatively, if you'd prefer not to spend any more time on it just yet, please open a separate issue that we can evaluate before potentially proceeding with a PR
  3. +1
  4. It's early days but we hope that service connections will be useful in Spring Cloud and beyond. For example, Spring Cloud Bindings may be able to define service connections rather than property sources.
  5. Our auto-configuration for Redis depends on Spring Data Redis as it uses org.springframework.data.redis.connection.lettuce.LettuceConnectionFactory or org.springframework.data.redis.connection.jedis.JedisConnectionFactory. The property prefix (spring.data.redis) and package names are intended to reflect this dependency
  6. Hazelcast's an interesting one as we lean heavily on Hazelcast's configuration files and have very little in the way of our own configuration properties. It would be good to support service connections with Hazelcast, but I'm not yet sure how that should be implemented. I've opened Provide service connection support for Hazelcast #34778 so that we can start to explore things. If you have any suggestions, they'd definitely welcome on that issue.

@eddumelendez
Copy link
Contributor Author

I will be submitting a PR for Redpanda :)

@wilkinsona thanks for the explanation! it all makes sense

@philwebb philwebb added the theme: containers Testcontainers and Docker Compose features label Apr 17, 2023
@eddumelendez
Copy link
Contributor Author

Before raising any PR. Does activemq and otlp service connections would be welcome? I think so but just wanted to confirm due to there is no specific testcontainers module but at least for otlp there is an official docker image.

@wilkinsona
Copy link
Member

Thanks for the offer, @eddumelendez. We're aware of a few places where service connection support is missing. There's the two that you've already mentioned, Apache Artemis, and there may well be others. Pull requests would be welcome if you have the time. Thank you. Just to set expectations, with RC1 being released tomorrow, there's a high chance that any new support won't land until 3.2 now.

@eddumelendez
Copy link
Contributor Author

Great! Yes, maybe we can create a list of those missing. You mean 3.1 next month, right?

So, probably this issue is already done. Please, feel free to close it and thanks for this feature again!

@wilkinsona
Copy link
Member

After tomorrow's RC1 we will try not to add any new features. It may not be a complete freeze, but we generally only fix bugs, improve the documentation, and amend existing functionality in the RC phase. That means that any new service connection support may have to wait until work in 3.2 begins in the summer once 3.1 has shipped.

So, probably this issue is already done. Please, feel free to close it and thanks for this feature again!

Thanks for the really timely feedback and your PRs. They're much appreciated.

@wilkinsona wilkinsona closed this as not planned Won't fix, can't repro, duplicate, stale Apr 19, 2023
@wilkinsona wilkinsona added status: superseded An issue that has been superseded by another and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another theme: containers Testcontainers and Docker Compose features
Projects
None yet
Development

No branches or pull requests

5 participants