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

Enable custom postgres config in devservice #25289

Merged

Conversation

newur
Copy link
Contributor

@newur newur commented May 1, 2022

I missed a way to configure Postgres in dev services. So far it is only possible for MariaDB/MySQL. So I added a similar approach.

Additionally, the dev services docu is scattered in quiet a lot of places. I created a new common page. similar to the other (much simpler) dev services. This is more or less independent from the code change - except for the added config example for postgres.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Thanks!

I added a couple of suggestions.
I'm not sure about the split of the guide but yeah maybe it makes sense.

But could you make the changes in two seperate commits of this PR as they are really unrelated (if you don't feel comfortable doing that with git, I can do it and keep your authorship).

@newur newur force-pushed the enable-custom-postgres-config-in-devservices branch from 4c226e0 to 32ee911 Compare May 5, 2022 18:28
@newur newur requested a review from gsmet May 5, 2022 18:29
@newur newur changed the title Enable custom postgres config in devservice; docu refactoring Enable custom postgres config in devservice May 5, 2022
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I had a closer look at this one and setCommand() is coming from GenericContainer so it is supported by all the databases.
Thus I think it should be made a global config and not part of this container properties thing.

Let's wait until #25530 is in. Then it should be easier to add an Optional<String> command to the config and consume it in all the *DevServicesProcessor.

@newur
Copy link
Contributor Author

newur commented May 12, 2022

I had a closer look at this one and setCommand() is coming from GenericContainer so it is supported by all the databases. Thus I think it should be made a global config and not part of this container properties thing.

Let's wait until #25530 is in. Then it should be easier to add an Optional<String> command to the config and consume it in all the *DevServicesProcessor.

I am not sure if I got you correctly here. You are right that setCommand() is coming from the GenericContainer. However, this overrides the Docker specific start command. I am not sure if in general all databases accept configuration parameters in this way.

If all database support it (or in general the user should have more controll over the dev startup) a global config sounds very good. +1

@gsmet
Copy link
Member

gsmet commented May 12, 2022

Well, you're not adding parameters to the database, you're changing the command used to start the container, be it for PostgreSQL or the others.

@newur
Copy link
Contributor Author

newur commented May 12, 2022

Right, this is what I said. Just wanted to raise my concern about the statement

it is supported by all the databases

@gsmet
Copy link
Member

gsmet commented May 12, 2022

Ah OK, I misunderstood what you said. My point is that even if you cannot push config parameters, well, it's optional so you're not forced to use it. And you might have other reasons to override the command.

I think it's better than introducing something specific to PostgreSQL given it's a general thing.

@gsmet
Copy link
Member

gsmet commented May 13, 2022

@newur I merged #25530 . Would you revisit this with a Optional<String> command in the config consumed by all the databases?

@newur newur force-pushed the enable-custom-postgres-config-in-devservices branch from 32ee911 to b7108b5 Compare May 20, 2022 15:38
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Perfect, thanks. Sorry for the delay in reviewing!

@gsmet gsmet merged commit 592e052 into quarkusio:main Jun 13, 2022
@quarkus-bot quarkus-bot bot added this to the 2.11 - main milestone Jun 13, 2022
@gsmet gsmet modified the milestones: 2.11 - main, 2.10.0.Final Jun 13, 2022
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.

None yet

2 participants