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

Add support to configuration file per profile #11145

Conversation

marcelorubim
Copy link
Contributor

@marcelorubim marcelorubim commented Aug 1, 2020

As described on issue #6810, this change adds the support to use a configuration file per-profile.

The user can have an application.properties with general configuration and one file for each profile, like application-dev.profile for dev profile.

Closes #6810

@famod
Copy link
Member

famod commented Aug 2, 2020

@radcortez This might be of interest to you. I am wondering whether this has any impact on #9895 / smallrye/smallrye-config#363.

@radcortez
Copy link
Member

Sort of.

These are two different things, since Quarkus way to define profiles as you know is to list them all in the same file. For a multi-profile, multi-file approach we would need to look as many files as profiles defined, not just an additional file.

@famod
Copy link
Member

famod commented Aug 3, 2020

Ok, thanks for your feedback. I just wanted to make sure that this not "diametrical" to what you have in mind for smalrye-config in context of #9895.

@radcortez
Copy link
Member

BTW, I've already added support for profile aware microprofile-config.properties files, which is coming in 2.0. For Quarkus, probably it would still require some custom implementation since the files names are not the same.

smallrye/smallrye-config@96bada1

Anyway, we probably have to look into making Quarkus / SmallRye Config more consistent and off load most of the work to SmallRye Config.

@marcelorubim
Copy link
Contributor Author

Nice! Maybe I should already add the same support for the MicroProfile files per profile in this PR.

@radcortez
Copy link
Member

Ideally we should support both at the same time. It would look weird to support one type of file and not the other, even considering that most people will use the application.properties file.

@marcelorubim
Copy link
Contributor Author

I included the treatment for MicroProfile files per profile as well.

@Ladicek
Copy link
Contributor

Ladicek commented Aug 18, 2020

I really don't think I'm the one who should review this, but FWIW, this LGTM :-)

(I actually prefer this style of config profiles over the %profile.whatever style Quarkus has. Thorntail had it right ;-) )

@famod
Copy link
Member

famod commented Aug 18, 2020

@marcelorubim it seems you included an unrelated commit (awssdk update).
Please also use rebase to update PR branches, not vanilla merge. Thank you very much!

@marcelorubim
Copy link
Contributor Author

@marcelorubim it seems you included an unrelated commit (awssdk update).
Please also use rebase to update PR branches, not vanilla merge. Thank you very much!

Sorry! I did some mess while rebasing. Now it's fixed! Thanks for warning me!

@@ -54,19 +56,25 @@

public static final class InJar extends ApplicationPropertiesConfigSource {
public InJar() {
super(openStream(), 250);
super(openStream(null), 260);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we keep the same ordinal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to follow up your sugestions.

I just keet the same pattern to add +10 for each one. But you're right. I would be better to not change the ordinal and only add +1.

}
return is;
}
}

public static final class InFileSystem extends ApplicationPropertiesConfigSource {
public InFileSystem() {
super(openStream(), 260);
super(openStream(null), 280);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we keep the same ordinal?

@@ -123,10 +114,9 @@ public static void addSourceProviders(SmallRyeConfigBuilder builder, Collection<
}

static class EnvConfigSource implements ConfigSource, Serializable {
static final Pattern REP_PATTERN = Pattern.compile("[^a-zA-Z0-9_]");
Copy link
Member

Choose a reason for hiding this comment

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

This change doesn't seem necessary.

@radcortez
Copy link
Member

radcortez commented Aug 18, 2020

Since we are adding new sources with new ordinals, there is a chance (low, but it is there) that user custom sources (if in the range of the used ordinals), conflict with the new ones. Everything will work, but maybe some configuration resolution may yield different results.

I would probably stick to use the same ordinal for the original sources and only increase the ordinal by +1 for the profile based properties. The chance will always be there, but it will make it even lower.

@marcelorubim
Copy link
Contributor Author

@gsmet @geoand could you please take a look at this PR?

@geoand
Copy link
Contributor

geoand commented Aug 21, 2020

I'll take a look some time next week. There is a ton of work that has pilled up

@geoand
Copy link
Contributor

geoand commented Aug 24, 2020

Question:

What happens with this PR when a user add %dev.quarkus.some.key=value to application-dev.properties?
What happens if the user adds %test.quarkus.some.key=value?

@marcelorubim
Copy link
Contributor Author

marcelorubim commented Aug 24, 2020

Question:

What happens with this PR when a user add %dev.quarkus.some.key=value to application-dev.properties?

If the user adds %dev.quarkus.some.key=value and quarkus.some.key=value1, the config with the profile prefix will be one loaded.

What happens if the user adds %test.quarkus.some.key=value?

It will be ignored, as the file is only loaded for the dev profile

@geoand

@geoand
Copy link
Contributor

geoand commented Aug 24, 2020

Question:
What happens with this PR when a user add %dev.quarkus.some.key=value to application-dev.properties?

If the user adds %dev.quarkus.some.key=value and quarkus.some.key=value1, the config with the profile prefix will be one loaded.

So essentially a user can continue using the profile prefix inside the profile specific file?

What happens if the user adds %test.quarkus.some.key=value?

It will be ignored, as the file is only loaded for the dev profile

I guess I wasn't clear. My question was what happens if %test.quarkus.some.key=value is added to application-dev.properties and then the dev profile is active.

@marcelorubim
Copy link
Contributor Author

So essentially a user can continue using the profile prefix inside the profile specific file?

Yes. I will be kind of redundant, but he can.

I guess I wasn't clear. My question was what happens if %test.quarkus.some.key=value is added to application-dev.properties and then the dev profile is active.

Quarkus will not load the application-dev.properties when the test profile is active, so the value will be ignored.

And during the dev profile, it will work the same as the default application.properties. Only the configurations without prefix or with the dev profile prefix will be loaded.

@geoand
Copy link
Contributor

geoand commented Aug 24, 2020

OK, thanks.

I am +0 on this TBH (because we might be introducing too many ways to do the same thing), so I'll let others approve it.

@famod
Copy link
Member

famod commented Oct 9, 2020

@geoand

so I'll let others approve it.

Is there anyone you'd suggest? Unfortunately this has stalled for over a month...

@geoand
Copy link
Contributor

geoand commented Oct 9, 2020

I'd propose sending an email to the mailing list with a link to this to get more opinions

@YunaBraska
Copy link

This is awesome but I think the changes are also needed for yaml configs like on io.quarkus.config.yaml.runtime.ApplicationYamlProvider
And second what's still missing for my use case coming from spring boot is to configure multiple profiles - but that's maybe a next topic since this pull request is opened for long enough. Can't wait to get it merged 👍

@radcortez
Copy link
Member

And second what's still missing for my use case coming from spring boot is to configure multiple profiles - but that's maybe a next topic since this pull request is opened for long enough. Can't wait to get it merged 👍

Multiple profiles are going to be available with #12789.

@AlbozDroid
Copy link

This is very important for us too, it helps to keep the config clean. Is there any plan for when to merge this?

@vsevel
Copy link
Contributor

vsevel commented Oct 30, 2020

hello, I think this is a good idea. here is a use case: https://groups.google.com/g/quarkus-dev/c/xCU6Yvw__ks

@radcortez
Copy link
Member

There is also some work going to support additional locations:
smallrye/smallrye-config#431

Base automatically changed from master to main March 12, 2021 15:54
@gsmet
Copy link
Member

gsmet commented Apr 1, 2021

@radcortez didn't you implement it now?

@radcortez
Copy link
Member

Yes.

Sorry @marcelorubim. This is now provided by SmallRye Config as part of the MicroProfile Config 2.0 work (since this was added into the spec). Thank you for your effort.

@radcortez radcortez closed this Apr 1, 2021
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Apr 1, 2021
@famod
Copy link
Member

famod commented Apr 1, 2021

@radcortez Isn't Quarkus still on SmallRye Config 1.x? AFAICS, SmallRye Config 2.x is based on MicroProfile Config 2.0, right?

@radcortez
Copy link
Member

@radcortez Isn't Quarkus still on SmallRye Config 1.x? AFAICS, SmallRye Config 2.x is based on MicroProfile Config 2.0, right?

Yes, but I back ported all the features that didn't have any API changes to the 1.x branch.

@famod
Copy link
Member

famod commented Apr 1, 2021

Ah, I see. Do you remember in which PR you did that? I would like to assign this issue to a milestone and temove triage/invalid. Thanks!

@radcortez
Copy link
Member

#15282

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple per-profile configuration files or includes
9 participants