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

Support Multiple Profiles. #12841

Closed
wants to merge 1 commit into from
Closed

Conversation

radcortez
Copy link
Member

No description provided.

@gastaldi gastaldi added this to the 1.10 - master milestone Oct 21, 2020
@dmlloyd
Copy link
Member

dmlloyd commented Oct 21, 2020

Was there a design discussion about supporting multiple profiles? Can it be linked from here?

@radcortez
Copy link
Member Author

Sort of. There are multiple issues:

#9895
smallrye/smallrye-config#363
smallrye/smallrye-config#399

@famod
Copy link
Member

famod commented Oct 21, 2020

WDYT about extending this as well?

For backward compatibility I guess a new method should be added and the old one should be marked deprecated?

/cc @geoand

@geoand
Copy link
Contributor

geoand commented Oct 21, 2020

WDYT about extending this as well?

For backward compatibility I guess a new method should be added and the old one should be marked deprecated?

/cc @geoand

I was not even aware of this (having been off doing some else for some time now), but in any case I would leave the test profile as is for now.
If this feature seems too appealing to miss, we can always add it later on.

@gastaldi
Copy link
Contributor

@radcortez I've seen you added some improvements to the SmallRye Config master. Are you going to release a new version and update this PR or would that happen in a separate PR?

@radcortez
Copy link
Member Author

I was thinking on doing that separate, but maybe it will be better to do it in this PR, so all the related changes are done together. I'll do a release and update this. Thanks!

@gastaldi
Copy link
Contributor

gastaldi commented Oct 23, 2020

Awesome! That would fix #2292 too. Thanks!

@gsmet
Copy link
Member

gsmet commented Oct 27, 2020

@dmlloyd I have a vague recollection of you having some reservations about adding this feature in the past. Am I wrong?

@dmlloyd
Copy link
Member

dmlloyd commented Oct 27, 2020

I recall there was a lengthy conversation. What use cases are being solved by this solution? Having just a PR with no specific instigating issue (with a use case) is not really our normal modus operandi at all, is it?

One concern about the implementation is that having more than one profile active at once means that there needs to be clear & specific rules for how they interplay; I think there is a lot of chance for confusion here. Also, if the profile is used for more than just configuration, then everything using profiles for any purpose has to be able to support mixing & matching in some sensible way; let's make sure we understand what does and will use profiles (maybe we can say it'll always only be configuration).

I'm concerned that this feature targets a particular use case that could be better solved some other less confusing way.

@radcortez
Copy link
Member Author

One of the main use cases is when you have configuration that is common between profiles, but not with the main profile.

For instance, lets say we have the following config:

quarkus.datasource.jdbc.url=jdbc:postgresql://localhost:5432/database
quarkus.datasource.db-kind=postgresql
quarkus.datasource.username=database

%dev.quarkus.datasource.jdbc.url=jdbc:h2:mem:mydb
%dev.quarkus.datasource.db-kind=h2
%dev.quarkus.datasource.username=sa

%test.quarkus.datasource.jdbc.url=jdbc:h2:mem:mydb
%test.quarkus.datasource.db-kind=h2
%test.quarkus.datasource.username=sa

Because we want to override the prod profile, we have to do it for both dev and test, duplicating the configuration.

This seems to be a recurring topic:
https://groups.google.com/u/1/g/quarkus-dev/c/ORyOZ7Dzm6o/m/Ow0H1p-5BQAJ
https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/Question.20about.20properties.20file
https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/multiple.20profiles
https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/Config.20Profiles.20in.20MP.20Config

When multiple profiles are active, the rules for profile configuration are exactly the same. If two profiles define the same configuration, then the last listed profile in the profile configuration value has priority.

For what I could see, the only cases where we use the config profiles is for configuration of course and for a few Arc annotations to add / removed beans based on configuration. The Spring Cloud Config Source also uses it, and we can update it to use multiple ones, since Spring Cloud does support multiple profiles as well.

@dmlloyd
Copy link
Member

dmlloyd commented Oct 27, 2020

One thought I had is that if you had common configuration in one profile and then the more specific in another, there would be at least one (and possibly several) combination(s) of profiles that would give you an invalid (or even dangerous) configuration. This seems to be a significant weakness of enabling multiple profiles.

One way to solve the use case without supporting multiple profiles (and thus enabling this possible problem) would be to allow one profile to extend another, so that the config system would look up the most specific profile first and then recurse to the less specific profiles. This way, to make an example, test could extend dev, and there are no "unsafe" combinations possible. WDYT?

@radcortez
Copy link
Member Author

radcortez commented Oct 27, 2020

Sounds reasonable.

@famod you were one of the original requesters for this feature. Is there any other use case that multiple profiles should cover?

@famod
Copy link
Member

famod commented Oct 27, 2020

Is there any other use case that multiple profiles should cover?

Dynamic selection at build time and/or runtime, e.g. -Dquarkus.profile=foo,bar. This way you don't need to write down each and every thinkable combination in application.properties (or .yaml).

TBH, you can always shoot yourself in the foot by configuring stupid things, so IMO I wouldn't deny users the possibility to select multiple profiles for just a small risk reduction.
I mean, the principle is very well known from Spring and people seem to like it (btw, #9895 has nine "likes") and it seems to come up every now and then also on Zulip.

That being said, "profile extensions" would be the next best thing and I can even think of scenarios where both approaches could co-exist nicely to increase developer joy.

@gsmet gsmet removed this from the 1.10 - master milestone Nov 10, 2020
@radcortez
Copy link
Member Author

How are we thinking to express profile extensions?

@dmlloyd
Copy link
Member

dmlloyd commented Nov 12, 2020

Maybe using a configuration key quarkus.parent-profile or something like that?

@radcortez
Copy link
Member Author

Sounds good. I'll write a prototype in SR Config.

@radcortez radcortez closed this Jan 4, 2021
@radcortez radcortez deleted the srconfig-192 branch January 4, 2021 14:27
@ghost ghost added the triage/invalid This doesn't seem right label Jan 4, 2021
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.

6 participants