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

Rewrite BuildTimeRunTime and RunTime Defaults ConfigSource #26802

Merged
merged 1 commit into from
Aug 9, 2022

Conversation

radcortez
Copy link
Member

@radcortez radcortez commented Jul 18, 2022

Currently, there are a few issues with the BuildTimeRunTime values and the RunTime Default values:

  • The source and values are generated directly in the Config class. In Dev mode the sources are not regenerated, meaning that lookups or injections to ConfigRoot classes keep the old values. This is not an issue in most cases because these configurations are used during build time in the extensions. Still, users can observe different values (or if somewhere in the code, we look up the property directly). The case of quarkus.tls.trust-all which is marked build-time, but it can be changed during runtime.
  • Do not override BUILD_AND_RUN_TIME_FIXED properties #20932 changed the BuildTime and RunTime values source to have the max original value. This prevented override of such configurations. This was not very visible because these configurations are primarily used in build time in the extensions, but users can observe different values.
  • Defaults values for BuildTime and RunTime could still be overridden since the source is set with a low ordinal.
  • The report of such values being overridden was inaccurate due to all of the above.

Changes in the PR:

  • The BuildTime and RunTime values and the RunTime Default values are now written to an external resource. This fixes the issue with reloading values in DEV mode.
  • The BuildTime and RunTime default values are now merged with the recorded user values.
  • The source gets max priority to disallow overring values. This prevents users from seeing different values if they look up a configuration directly or inject a ConfigRoot class.
  • The recorded RunTime Default values are now merged with the Build Item RunTime Default values
  • Checking for overrides warnings takes into account the source of the configuration

A few extra considerations:

  • We now instantiate the source twice (for static and runtime). There are other cases where we double instance sources. I think we can improve this generally by just reusing the builder from static to runtime.
  • These sources are now a resource for the native image. We can probably still generate a class with all the values, but I wanted to keep the same code and behavior between dev and native.
  • Due to being a single source now and the defaults being in higher priority, it should require fewer lookups to create the ConfigRoot classes.

@radcortez radcortez marked this pull request as draft July 18, 2022 23:53
@radcortez radcortez force-pushed the fix-26156 branch 4 times, most recently from 65c2a0f to 4879858 Compare July 20, 2022 16:48
@radcortez radcortez changed the title Properly reload BuildTimeRunTime values in Dev mode Rewrite BuildTimeRunTime ConfigSource Jul 20, 2022
@radcortez radcortez marked this pull request as ready for review July 20, 2022 17:02
@quarkus-bot

This comment has been minimized.

@radcortez radcortez marked this pull request as draft July 20, 2022 23:54
@radcortez radcortez force-pushed the fix-26156 branch 2 times, most recently from af80dc9 to 6d135ab Compare July 21, 2022 17:10
@radcortez radcortez marked this pull request as ready for review July 21, 2022 17:22
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@radcortez radcortez changed the title Rewrite BuildTimeRunTime ConfigSource Rewrite BuildTimeRunTime and RunTime Defaults ConfigSource Jul 26, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 27, 2022

Failing Jobs - Building 56160c0

Status Name Step Failures Logs Raw logs
JVM Tests - JDK 11 Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17
✔️ JVM Tests - JDK 18

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: extensions/smallrye-reactive-messaging-amqp/deployment 
! Skipped: integration-tests/reactive-messaging-amqp 

📦 extensions/smallrye-reactive-messaging-amqp/deployment

io.quarkus.smallrye.reactivemessaging.amqp.SecuredAmqpTest.test line 28 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with io.quarkus.smallrye.reactivemessaging.amqp.SecuredAmqpTest was not fulfilled within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

@radcortez radcortez requested review from dmlloyd and geoand July 28, 2022 15:19
@geoand
Copy link
Contributor

geoand commented Jul 29, 2022

👍🏼

Seems reasonable to me.

@gsmet
Copy link
Member

gsmet commented Aug 8, 2022

@yrodiere given you had a look at the config code lately, I wonder if you could have a quick look at this one? Feel free to say no if you're not familiar at all with this code :).

Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

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

@yrodiere given you had a look at the config code lately, I wonder if you could have a quick look at this one? Feel free to say no if you're not familiar at all with this code :).

I'm afraid my experience with that code is quite limited, but from that (limited) perspective, the changes look good.

In any case, replacing bytecode generation with a simple text resource will get a +1 from me anytime 😁 It's much easier to debug.

@gsmet gsmet merged commit 87add08 into quarkusio:main Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants