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 loading additional configurations locations. #431

Merged
merged 1 commit into from
Nov 25, 2020

Conversation

radcortez
Copy link
Member

@radcortez radcortez commented Oct 25, 2020

A lot of users have been requesting the ability to load external configuration files:
quarkusio/quarkus#12419
quarkusio/quarkus#11273
quarkusio/quarkus#9869

Also it seems to be a recurring topic in Zulip:
https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/application.2Eproperties.20file.20from.20file.20system
https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/External.20properties
https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/Shared.2Fglobal.20application.2Eproperties.20in.20Maven.20multimodule
https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/Additional.20config.20properties.20at.20build.20time
https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/Quarkus.20configuration

The configuration smallrye.config.locations sets additional locations to load configurations. These can be a file, a directory contents, a class path resource or an http resource. Additional locations are only added if the config is set. If not, there is no change on how the config behaves. Since this requires the config to be the factories are auto registered with the Config object, so no additional setup is required.

This is just a prototype and requires some additional polishing.

@radcortez
Copy link
Member Author

radcortez commented Oct 25, 2020

@xtaixe, @hrstoyanov @mklueh any chance for any of you to try this out and give me some feedback?

You should be able to use this with Quarkus 1.9.1, if you build a local version of SR Config with this PR and updates to the following dependencies:

<dependency>
  <groupId>io.smallrye.config</groupId>
  <artifactId>smallrye-config</artifactId>
  <version>1.9.4-SNAPSHOT</version>
</dependency>

Thanks!

@xtaixe
Copy link

xtaixe commented Oct 26, 2020

@radcortez tested. It works for me with a @QuarkusTest and with the runner jar, but I couldn't test with native because when trying to build the native image I get

Error: could not find target method: static java.lang.Class io.quarkus.runtime.configuration.Substitutions$Target_ConfigMappingLoader.loadClass(java.lang.String,byte[])
com.oracle.svm.core.util.UserError$UserException: could not find target method: static java.lang.Class io.quarkus.runtime.configuration.Substitutions$Target_ConfigMappingLoader.loadClass(java.lang.String,byte[])
	at com.oracle.svm.core.util.UserError.abort(UserError.java:68)
	at com.oracle.svm.hosted.substitute.AnnotationSubstitutionProcessor.findOriginalMethod(AnnotationSubstitutionProcessor.java:637)

which I'm not getting without the changes to test this. Just FYI.

As a feature, this would work for us.

Thanks for starting the work on this.

@radcortez
Copy link
Member Author

@xtaixe Thanks for trying this out.

Yes, the latest master also changed a few things in the API that affect the native image generation, but these is already handled here: quarkusio/quarkus#12501

So no worries about the native image, this will be fully working once released.

@radcortez radcortez force-pushed the additional-sources branch 2 times, most recently from a6a9805 to 05c5070 Compare October 29, 2020 12:24
@radcortez
Copy link
Member Author

@dmlloyd any thoughts?

@dmlloyd
Copy link
Contributor

dmlloyd commented Oct 29, 2020

It seems OK in principle. We would absolutely have to document the exact rules for which configuration files take precedence and in what order though. There could be a lot of user confusion in this regard.

@radcortez
Copy link
Member Author

I agree. I think that loading additional sources this way should never take precedence over system properties or environment variables. It should be ok to haver an higher priority than application.properties or microprofile-config.properties.

For order, it may be a little trickier. If you set config_ordinal in the source it would be very well defined. If not, it falls in our rule of ordering by ordinal and then by name (which gives you some predictability, but maybe not what you want). We could add a extra rule to take into account the definition order in the configuration.

@Emily-Jiang
Copy link
Contributor

There is a similar issue in the MicroProfile Config github. I think it is a good idea. I agree that we can only process .properties files to start with.

I am thinking this property smallrye.config.location could be specified in a config source. Basically, you can put smallrye.config.location in your microprofile-config.properties. In this case, the ordinal in the properties files should inherit the ordinal of the config source, where the property is specified if the property config_ordinal is not specified in the property files.

@radcortez
Copy link
Member Author

I am thinking this property smallrye.config.location could be specified in a config source.

Yes, I added support to set it up with any other valid source.

Basically, you can put smallrye.config.location in your microprofile-config.properties. In this case, the ordinal in the properties files should inherit the ordinal of the config source, where the property is specified if the property config_ordinal is not specified in the property files.

This might be a little tricky. If the additional file has the same ordinal of the defining source, what happens if both files define the same property name with different values? Right now we do a second sort by the ConfigSource name, but it is not ideal.

@Emily-Jiang
Copy link
Contributor

This might be a little tricky. If the additional file has the same ordinal of the defining source, what happens if both files define the same property name with different values? Right now we do a second sort by the ConfigSource name, but it is not ideal.

If they have the same ordinal, the order is undefined. In the impl, we could say the additional file overrides the parent one as the additional file is somewhere and can be seen to override the property value in the parent.

@dmlloyd
Copy link
Contributor

dmlloyd commented Oct 29, 2020

This might be a little tricky. If the additional file has the same ordinal of the defining source, what happens if both files define the same property name with different values? Right now we do a second sort by the ConfigSource name, but it is not ideal.

If they have the same ordinal, the order is undefined. In the impl, we could say the additional file overrides the parent one as the additional file is somewhere and can be seen to override the property value in the parent.

A reasonable implementation rule would have been to order first by ordinal, and then by load order (our container implementations have always had a stable class loading order). The second sort by config source name is really not functionally useful, being intended only to bring stability to the sort, but I think that it is actually detrimental in practice: it removes the much more useful stability of the initial load order that otherwise would have taken effect.

@radcortez
Copy link
Member Author

Sounds reasonable. I don't think anyone is relying on the name sort, so I'll make some changes to use the loading order sort.

@radcortez radcortez force-pushed the additional-sources branch 3 times, most recently from 11daf85 to 3866585 Compare November 2, 2020 23:57
@Emily-Jiang
Copy link
Contributor

The PR sounds reasonable to me!

@radcortez radcortez marked this pull request as ready for review November 3, 2020 11:25
@radcortez
Copy link
Member Author

@dmlloyd are you ok with this? :)

@dmlloyd
Copy link
Contributor

dmlloyd commented Nov 12, 2020

There are probably more cleanups possible, because it still seems to me that we're doing a lot of unneeded hopping back and forth between forms of URI and other APIs, but I didn't see anything else that is actually problematic.

@radcortez
Copy link
Member Author

There are probably more cleanups possible, because it still seems to me that we're doing a lot of unneeded hopping back and forth between forms of URI and other APIs, but I didn't see anything else that is actually problematic.

I'll try to have another look, but in the load of the ConfigSource itself we need to keep URL because a classloader resource may contain a custom URLHandler.

@radcortez
Copy link
Member Author

Also, it could probably be improved by adjusting the API in ClassPathUtils.

@hrstoyanov
Copy link

Is this going to be part of 1.10 release?

@radcortez
Copy link
Member Author

Is this going to be part of 1.10 release?

Unlikely, since Quarkus 1.10 is already in the final preparation stage, we usually refrain to introduce major updates to dependencies.

Still, SR Config should be completely compatible once we release, so you can probably update the dependency manually in your project to use the feature until it is GA in Quarkus itself.

Sorry for taking so much time :(

@radcortez
Copy link
Member Author

@dmlloyd are you ok with this? :)

@radcortez radcortez force-pushed the additional-sources branch 5 times, most recently from 215e5f9 to c92bcf2 Compare November 25, 2020 19:38
@radcortez
Copy link
Member Author

In the meanwhile, I've made some additional changes: I've moved the loading code to its own class, so it can be used anywhere and not just in the Factory.

@radcortez radcortez merged commit 6adbe8e into smallrye:master Nov 25, 2020
@radcortez radcortez deleted the additional-sources branch November 25, 2020 23:42
@radcortez radcortez added this to the 1.10.0 milestone Dec 7, 2020
@radcortez radcortez changed the title Prototype to support loading additional configurations locations. Support loading additional configurations locations. Dec 7, 2020
@hrstoyanov
Copy link

@radcortez Now that Quarkus 1.11.CR1 is out, how can I use this in Quarkus? Is there a better version of this tutorial? Does this work with YAML configurations? A simple example for alternative YAML file location?

I am also interested in how to point Quarkus to alternative configuration during unit testing

@radcortez
Copy link
Member Author

Hi @hrstoyanov, please refer to this:
https://smallrye.io/docs/smallrye-config/config/config.html#locations

Just use smallrye.config.locations to set alternate locations. This works as any other configuration property, so you can use it as an env variable, system property, etc. YAML should also be supported.

Let me know if you need any help.

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

Successfully merging this pull request may close these issues.

5 participants