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.

Sign up for GitHub

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

Jump to bottom

K8S service binding support for Reactive SQL Clients #25657

Merged
merged 2 commits into from May 20, 2022
Merged

K8S service binding support for Reactive SQL Clients #25657

merged 2 commits into from May 20, 2022

Conversation

tsegismont
Copy link
Contributor

This commit introduces support for K8S service binding when using Reactive SQL Clients.

@tsegismont tsegismont requested review from gsmet, cescoffier and geoand and removed request for gsmet May 18, 2022 14:10
@tsegismont
Copy link
Contributor Author

@geoand in addition to the feature implementation, we could reduce code duplication with a small refactoring. Instead of JdbcDatasourceUtil static methods, we could extend ReactiveDatasourceServiceBindingConfigSourceFactory (sorry for the name!) and override only specific methods like urlPropertyName and formatUrl.

Then the same code base would handle both jdbc and reactive config sources when using k8s service bindings.

Wdyt?

@quarkus-bot

This comment has been minimized.

Sign in to view
@geoand
Copy link
Contributor

geoand commented May 18, 2022

Sounds like a good idea to me 🙂

@quarkus-bot

This comment has been minimized.

Sign in to view
@geoand
Copy link
Contributor

geoand commented May 19, 2022

Also, please rebase the PR onto a main in order to pick up a CI fix

@tsegismont tsegismont force-pushed the service_binding_reactive_sql_clients branch from 72c5943 to 287c81f Compare May 19, 2022 14:36
@tsegismont
Copy link
Contributor Author

@geoand done

@geoand
Copy link
Contributor

geoand commented May 19, 2022

Great! I'll have a look tomorrow morning

cescoffier
cescoffier approved these changes May 19, 2022
View reviewed changes
Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

LGTM, but my knowledge about SBO is quite superficial.

@quarkus-bot

This comment has been minimized.

Sign in to view
geoand
geoand reviewed May 20, 2022
View reviewed changes
Copy link
Contributor

@geoand geoand left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for this, LGTM!

I only have one comment: Can you squash the first and second commits please?
We try to avoid having commits that would break the build.
That way this PR would only have two commits: The first one that adds the Service Binding support for the reactive clients and the other one performing the refactoring.

@tsegismont
K8S service binding support for Reactive SQL Clients
a6e4a57 
This commit introduces support for K8S service binding when using Reactive SQL Clients.
@tsegismont
Refactored ServiceBindingConfigSource creation for jdbc/reactive data…
af60d0b 
…sources

This commit introduces a base class for `ServiceBindingConfigSource` creation: `DatasourceServiceBindingConfigSourceFactory`.

This abstract class has a `Jdbc` and a `Reactive` implementation.

The `formatUrl` method can be overridden in db client extensions in order to handle specific case, e.g.:

- MariaDB JDBC extension uses `mysql` service binding type but generates a URL with `mariadb` prefix
- PostgreSQL JDBC extension handles specific cloud configurations using JDBC URL options
@tsegismont tsegismont force-pushed the service_binding_reactive_sql_clients branch from 287c81f to af60d0b Compare May 20, 2022 08:06
@tsegismont
Copy link
Contributor Author

@geoand done

@tsegismont
Copy link
Contributor Author

I've added the release/noteworthy label

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 20, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented May 20, 2022

Failing Jobs - Building af60d0b

Status Name Step Failures Logs Raw logs
✖ Devtools Tests - JDK 11 Build Failures Logs Raw logs
✖ Devtools Tests - JDK 17 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Devtools Tests - JDK 11 #

- Failing: integration-tests/devtools

📦 integration-tests/devtools

✖ io.quarkus.platform.catalog.RegistrySnapshotCatalogCompatibilityTest.testRegistrySnapshotPlatformCatalog line 30 - More details - Source on GitHub

java.lang.IllegalStateException: Failed to locate the dev tools config file (.quarkus/config.yaml)
	at io.quarkus.devtools.testing.registry.client.TestRegistryClient.<init>(TestRegistryClient.java:33)
	at io.quarkus.devtools.testing.registry.client.TestRegistryClientFactory.buildRegistryClient(TestRegistryClientFactory.java:32)

⚙️ Devtools Tests - JDK 17 #

- Failing: integration-tests/devtools

📦 integration-tests/devtools

✖ io.quarkus.platform.catalog.RegistrySnapshotCatalogCompatibilityTest.testRegistrySnapshotPlatformCatalog line 30 - More details - Source on GitHub

java.lang.IllegalStateException: Failed to locate the dev tools config file (.quarkus/config.yaml)
	at io.quarkus.devtools.testing.registry.client.TestRegistryClient.<init>(TestRegistryClient.java:33)
	at io.quarkus.devtools.testing.registry.client.TestRegistryClientFactory.buildRegistryClient(TestRegistryClientFactory.java:32)

@geoand geoand merged commit 8022fe5 into quarkusio:main May 20, 2022
@quarkus-bot quarkus-bot bot added this to the 2.10 - main milestone May 20, 2022
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 20, 2022
@tsegismont tsegismont deleted the service_binding_reactive_sql_clients branch May 20, 2022 12:26
@michalvavrik michalvavrik mentioned this pull request Oct 31, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cescoffier cescoffier cescoffier approved these changes

@geoand geoand geoand approved these changes

@gsmet gsmet Awaiting requested review from gsmet

Assignees
No one assigned
Labels
area/persistence area/reactive-sql-clients release/noteworthy-feature
Projects
None yet
Milestone
2.10.0.CR1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tsegismont @geoand @cescoffier