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

Introduce new configuration framework and update SmallRye Config version #5387

Merged
merged 39 commits into from
Nov 23, 2019

Conversation

dmlloyd
Copy link
Member

@dmlloyd dmlloyd commented Nov 11, 2019

  • Fixes a variety of configuration related issues.
  • Changes the strategy for configuration reading so that all groups are read eagerly always (in the case of config roots) or as soon as a property within the group is detected (in the case of map values or leaves). This allows environment variables to override most configuration properties without requiring the property to be present in application.properties.
  • Use the SmallRye Config converter infrastructure to dramatically reduce the number of special cases related to configuration property analysis. Only nested maps generally need significantly specialized behavior now.
  • Explicitly defines more behaviors regarding empty value handling, default values for primitives, optional versus required, arrays, and collections.
  • Tighten rules about accessing run time configuration values from build steps.
  • Explicitly make run time configuration available to dev mode on deploy/start failure.
  • Make doc generation support arrays.
  • Use new config factory SPI to avoid issues around multiple ConfigProviderResolver instances.
  • Support optional @ConfigGroups.

Before graduating from draft:

  • SmallRye Config 1.4.0 1.4.1 release (@kenfinnigan) and PR update (@dmlloyd)
  • Tests for Map<String, Map<String, Xxx>> where Xxx is a leaf or config group (@machi1990)
  • Tests to verify environment var overlay works as advertised (@machi1990)
  • Verify that config documentation website generator works (@machi1990)
  • Documentation: describe default values for primitives (@dmlloyd)
  • Documentation: update extension author's guide describe eager config loading strategy and optional config groups (@dmlloyd)
  • Documentation: describe (for users) empty value handling, how to explicitly clear a property (@dmlloyd)
  • Documentation: list (for users) all the default configuration sources and their priority (@dmlloyd)
  • Documentation: update extension author's guide to clarify rules around run time configuration object handling (@dmlloyd)
  • Documentation: update extension author's guide to describe empty value handling in extensions (@dmlloyd)
  • Verify Quickstarts (@gastaldi)
  • Verify Docker build (@dmlloyd)
  • Verify native builds (@dmlloyd)

Please submit supplementary PRs to https://github.com/dmlloyd/quarkus/tree/config-updates-final.

@gsmet
Copy link
Member

gsmet commented Nov 13, 2019

@dmlloyd can you rebase on top of the latest CI fixes? Thanks!

@gsmet
Copy link
Member

gsmet commented Nov 13, 2019

@machi1990 was wondering if you could take these 2 ones:

  • Verify that config documentation website generator works
  • Verify Quickstarts

For the latter, we need first a JVM run but a native one could be beneficial too. If you can't do it locally, we can see with @cescoffier if we can do that on CI.

@dmlloyd
Copy link
Member Author

dmlloyd commented Nov 13, 2019

No problem. It's safe to say that I'm going to be rebasing about 5x a day - this PR touches a lot of things so it's pretty fragile. I just have to figure out why native stopped working first.

@machi1990
Copy link
Member

@machi1990 was wondering if you could take these 2 ones:

  • Verify that config documentation website generator works

Yes, I can do that. I ran a full build yesterday and did not see any issues. I'll run a diff of the generated docs a bit later and see if we have any rendering issue. I can un-archive https://github.com/machi1990/quarkus-generated-extension-config-docs to create a separate branch there with the result of this patch .

  • Verify Quickstarts

For the latter, we need first a JVM run but a native one could be beneficial too. If you can't do it locally, we can see with @cescoffier if we can do that on CI.

Sounds like a plan. I verified the JVM run on getting-started and the mqtt quickstarts yesterday. Will check for the others too. @gastaldi you were also checking the quickstarts?

@gastaldi
Copy link
Contributor

@machi1990 yes, I ran them with mvn clean test -Dquarkus.version=999-SNAPSHOT but the build fails with:

[INFO] Running org.acme.config.GreetingResourceTest
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.257 s <<< FAILURE! - in org.acme.config.GreetingResourceTest
[ERROR] testHelloEndpoint  Time elapsed: 0.006 s  <<< ERROR!
org.junit.jupiter.api.extension.TestInstantiationException: TestInstanceFactory [io.quarkus.test.junit.QuarkusTestExtension] failed to instantiate test class [org.acme.config.GreetingResourceTest]: io/quarkus/runtime/configuration/SimpleConfigurationProviderResolver
Caused by: java.lang.NoClassDefFoundError: io/quarkus/runtime/configuration/SimpleConfigurationProviderResolver
Caused by: java.lang.ClassNotFoundException: io.quarkus.runtime.configuration.SimpleConfigurationProviderResolver

I'm investigating why that happens.

@dmlloyd dmlloyd force-pushed the config-updates-final branch 2 times, most recently from 595bc00 to 9391f3e Compare November 13, 2019 16:12
@dmlloyd
Copy link
Member Author

dmlloyd commented Nov 13, 2019

Rebased BTW. Let's see how it flies. I also have another complete run going locally which will probably finish before CI.

@machi1990
Copy link
Member

@machi1990 yes, I ran them with mvn clean test -Dquarkus.version=999-SNAPSHOT but the build fails with:

[INFO] Running org.acme.config.GreetingResourceTest
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.257 s <<< FAILURE! - in org.acme.config.GreetingResourceTest
[ERROR] testHelloEndpoint  Time elapsed: 0.006 s  <<< ERROR!
org.junit.jupiter.api.extension.TestInstantiationException: TestInstanceFactory [io.quarkus.test.junit.QuarkusTestExtension] failed to instantiate test class [org.acme.config.GreetingResourceTest]: io/quarkus/runtime/configuration/SimpleConfigurationProviderResolver
Caused by: java.lang.NoClassDefFoundError: io/quarkus/runtime/configuration/SimpleConfigurationProviderResolver
Caused by: java.lang.ClassNotFoundException: io.quarkus.runtime.configuration.SimpleConfigurationProviderResolver

I'm investigating why that happens.

Okay thanks @gastaldi. Since you are checking the Quickstarts, I'll fully concentrate on doc generation part. :-)

@gastaldi
Copy link
Contributor

I found an issue while running the config-quickstart against this branch with mvn verify -Dnative:

[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.acme.config.NativeGreetingResourceIT
Executing [/home/ggastald/workspace/quarkus-quickstarts/config-quickstart/target/config-quickstart-1.0-SNAPSHOT-runner, -Dquarkus.http.port=8081, -Dtest.url=http://localhost:8081, -Dquarkus.log.file.path=target/quarkus.log]
javax.enterprise.inject.spi.DeploymentException: No config value of type [java.lang.String] exists for: greeting.message
	at io.quarkus.arc.runtime.ConfigRecorder.validateConfigProperties(ConfigRecorder.java:37)
	at io.quarkus.deployment.steps.ConfigBuildStep$validateConfigProperties100.deploy_0(ConfigBuildStep$validateConfigProperties100.zig:101)
	at io.quarkus.deployment.steps.ConfigBuildStep$validateConfigProperties100.deploy(ConfigBuildStep$validateConfigProperties100.zig:120)
	at io.quarkus.runner.ApplicationImpl.doStart(ApplicationImpl.zig:113)
	at io.quarkus.runtime.Application.start(Application.java:87)
	at io.quarkus.runtime.Application.run(Application.java:210)
	at io.quarkus.runner.GeneratedMain.main(GeneratedMain.zig:41)
Exception in thread "main" java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.runner.ApplicationImpl.doStart(ApplicationImpl.zig:177)
	at io.quarkus.runtime.Application.start(Application.java:87)
	at io.quarkus.runtime.Application.run(Application.java:210)
	at io.quarkus.runner.GeneratedMain.main(GeneratedMain.zig:41)
Caused by: javax.enterprise.inject.spi.DeploymentException: No config value of type [java.lang.String] exists for: greeting.message
	at io.quarkus.arc.runtime.ConfigRecorder.validateConfigProperties(ConfigRecorder.java:37)
	at io.quarkus.deployment.steps.ConfigBuildStep$validateConfigProperties100.deploy_0(ConfigBuildStep$validateConfigProperties100.zig:101)
	at io.quarkus.deployment.steps.ConfigBuildStep$validateConfigProperties100.deploy(ConfigBuildStep$validateConfigProperties100.zig:120)
	at io.quarkus.runner.ApplicationImpl.doStart(ApplicationImpl.zig:113)
	... 3 more
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 60.131 s <<< FAILURE! - in org.acme.config.NativeGreetingResourceIT

It works with 1.0.0.CR1 (quarkus-quickstarts master branch)

machi1990 added a commit to machi1990/quarkus-generated-extension-config-docs that referenced this pull request Nov 13, 2019
@dmlloyd
Copy link
Member Author

dmlloyd commented Nov 14, 2019

@gastaldi I think that problem might be the same as the native problem I'm seeing. I'm testing a fix now.

@machi1990
Copy link
Member

machi1990 commented Nov 14, 2019

@dmlloyd @gsmet

I did a full build and now I am going through the generated docs, I have noticed a couple of fixes need to be applied to the doc generator.

Those two are the major changes from I could quickly spot using https://github.com/machi1990/quarkus-generated-extension-config-docs/compare/5387-quarkus-pr-on-config-patch?expand=1#diff-94f75c83d58bea1e46b66680f9d83c11R11

I'll work on fixing them.

@dmlloyd dmlloyd force-pushed the config-updates-final branch from 9391f3e to 2287c29 Compare November 14, 2019 19:17
@dmlloyd
Copy link
Member Author

dmlloyd commented Nov 14, 2019

Thanks @machi1990.

machi1990 added a commit to machi1990/quarkus-generated-extension-config-docs that referenced this pull request Nov 14, 2019
@dmlloyd dmlloyd merged commit bc066bc into quarkusio:master Nov 23, 2019
@dmlloyd dmlloyd deleted the config-updates-final branch November 23, 2019 02:23
@gsmet gsmet added this to the 1.1.0 milestone Nov 23, 2019
machi1990 added a commit to machi1990/quarkus that referenced this pull request Nov 23, 2019
handle optional config group in doc generation

Fixes quarkusio#5703

Follows up quarkusio#5387
machi1990 added a commit to machi1990/quarkus that referenced this pull request Nov 24, 2019
handle optional config group in doc generation

Fixes quarkusio#5703

Follows up quarkusio#5387
machi1990 added a commit to machi1990/quarkus that referenced this pull request Nov 24, 2019
handle optional config group in doc generation

Fixes quarkusio#5703

Follows up quarkusio#5387
mmusgrov pushed a commit to mmusgrov/quarkus that referenced this pull request Dec 13, 2019
handle optional config group in doc generation

Fixes quarkusio#5703

Follows up quarkusio#5387
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.