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

Set quarkus.datasource.db-kind property in PostgreSql Service Binding #14328

Closed
wants to merge 1 commit into from

Conversation

johnpoth
Copy link

…rviceBindingConverter

Fixes #14327

Thanks !

@ghost
Copy link

ghost commented Jan 15, 2021

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should not end up with ellipsis (make sure the title is complete)
  • title should preferably start with an uppercase character (if it makes sense!)
  • title should not contain an issue number (use 'Fix Allow * expression for quarkus:add-extension #1234' in the description instead)

This message is automatically generated by a bot.

@johnpoth johnpoth changed the title fix (#14326): set quarkus.datasource.db-kind property in PostgreSqlSe… set quarkus.datasource.db-kind property in PostgreSqlSe… Jan 15, 2021
@johnpoth johnpoth changed the title set quarkus.datasource.db-kind property in PostgreSqlSe… Set quarkus.datasource.db-kind property in PostgreSql Service Binding Jan 15, 2021
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Good catch!

@geoand geoand added triage/backport? triage/invalid This doesn't seem right and removed triage/backport? labels Jan 15, 2021
@geoand
Copy link
Contributor

geoand commented Jan 15, 2021

Actually, I remember why this was not done in the first place and why it's wrong.

quarkus.datasource.db-kind is a build-time property. So you need to have it set during build, it doesn't have any effect when setting at runtime.

cc @gsmet

@geoand geoand closed this Jan 15, 2021
@gsmet
Copy link
Member

gsmet commented Jan 15, 2021

I think the only thing you can do is check the build time value and throw an error if it's not consistent.

@geoand
Copy link
Contributor

geoand commented Jan 15, 2021

I don't think that provides much value TBH

@johnpoth
Copy link
Author

@geoand, thanks!

@gsmet when creating a default datasource, do all database properties need to be set at build-time (password, username, url, db-kind...) or just the db-kind? While testing it seems it's the former

@geoand
Copy link
Contributor

geoand commented Jan 18, 2021

I am pretty sure you don't need the runtime properties (like the URL, the username etc.) set, but I could be mistaken and maybe you need some dummy values set

@johnpoth
Copy link
Author

@geoand gotcha. I see two ways of solving this:

1] Take quarkus.datasource.db-kind into account at runtime

2] Read the Service Bindings at build time as well as at runtime

If you'd like I can do an initial implementation

@geoand
Copy link
Contributor

geoand commented Jan 18, 2021

@geoand gotcha. I see two ways of solving this:

1] Take quarkus.datasource.db-kind into account at runtime

Unfortunately this is not an option: Quarkus needs to know a lot of stuff at buid time

2] Read the Service Bindings at build time as well as at runtime

If you'd like I can do an initial implementation

But if you do this, properties won't be available at runtime, no?

I might have missed this, but what is the original problem you are trying to solve?

@johnpoth
Copy link
Author

@geoand I was thinking of adding the kubernetes service binding as a config source at build time too. That way the user only has to use the service binding for a default datasource to be created and doesn't have to add something else to his config or command line. In our case, the user has to set quarkus.datasource.db-kind correctly. I think this would make the user experience better. Hope this makes sense, let me know what you think thanks !

@geoand
Copy link
Contributor

geoand commented Jan 19, 2021

Ah, now I think I understand.

So you basically want the following:

When a user has the quarkus-kubernetes-service-binding extension and quarkus-jdbc-postgresql, then have quarkus.datasource.db-kind set to postgresql automatically?

I think that it's a good idea. @gsmet how does that sounds to you?

@geoand
Copy link
Contributor

geoand commented Jan 19, 2021

I opened a draft PR showing the approach I mentioned above #14393.

@gsmet care to comment on that since you are the SME on all things datasource?

@johnpoth
Copy link
Author

@geoand exactly! Maybe if there are several datasources configured by service bindings then set the datasource name to the servicebinding name or something (but keeping a default datasource in simple use is nice to have IMO). Thanks !

@geoand
Copy link
Contributor

geoand commented Jan 19, 2021

Currently we are not implementing multiple database resources - that is a next thing we need to look into

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

quarkus.datasource.db-kind is not set when using PostgreSql ServiceBinding
3 participants