-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 first version of bootstrap config support #6913
Conversation
core/processor/src/main/java/io/quarkus/annotation/processor/generate_doc/ConfigPhase.java
Outdated
Show resolved
Hide resolved
e4aff68
to
b93af17
Compare
@@ -346,12 +346,13 @@ stages: | |||
parameters: | |||
poolSettings: ${{parameters.poolSettings}} | |||
expectUseVMs: ${{parameters.expectUseVMs}} | |||
timeoutInMinutes: 30 | |||
timeoutInMinutes: 35 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh oh. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well this was just done to be on the safe side since I added a new test :).
I didn't see any actual problem :P
core/deployment/src/main/java/io/quarkus/deployment/ExtensionLoader.java
Outdated
Show resolved
Hide resolved
core/deployment/src/main/java/io/quarkus/deployment/ExtensionLoader.java
Outdated
Show resolved
Hide resolved
core/deployment/src/main/java/io/quarkus/deployment/ExtensionLoader.java
Outdated
Show resolved
Hide resolved
core/deployment/src/main/java/io/quarkus/deployment/ExtensionLoader.java
Outdated
Show resolved
Hide resolved
core/deployment/src/main/java/io/quarkus/deployment/ExtensionLoader.java
Outdated
Show resolved
Hide resolved
...loyment/src/main/java/io/quarkus/deployment/configuration/RunTimeConfigurationGenerator.java
Outdated
Show resolved
Hide resolved
runTimePatternMap, combinator), bootstrapPatternMap, combinator); | ||
final ConfigPatternMap<Container> runTimeIgnored = ConfigPatternMap | ||
.merge(ConfigPatternMap.merge(buildTimePatternMap, | ||
buildTimeRunTimePatternMap, combinator), bootstrapPatternMap, combinator); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in the future we should have a separate bootstrapIgnored
as well, so we can log warnings about unrecognized configurations in the bootstrap config file(s) (this is the common case) while also being able to warn about ineffective config in the run time config (for example if a config source tries to configure itself, we can warn that the configuration property may not be given at that time). This depends on #5548.
core/deployment/src/main/java/io/quarkus/deployment/steps/MainClassBuildStep.java
Outdated
Show resolved
Hide resolved
core/deployment/src/main/java/io/quarkus/deployment/steps/MainClassBuildStep.java
Outdated
Show resolved
Hide resolved
@@ -37,6 +39,20 @@ public int compare(ConfigPhase firstPhase, ConfigPhase secondPhase) { | |||
return -1; | |||
} | |||
} | |||
case BOOTSTRAP: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh! This bit of code probably should not have been approved - instead the phases probably ought to have been reordered and the default comparison used.... but I guess that can be a fix for another day.
@dmlloyd I'll add my comments later on today or tomorrow morning. |
b93af17
to
b0a65f5
Compare
@dmlloyd The solution has been updated to incorporate your suggestion of moving the config loading into build steps and using build items that allow the build system to work out the proper order of things. I feel much better about this iteration - hopefully you will too 😆 |
@dmlloyd kind reminder 😁 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking better, I think the general approach seems OK.
core/deployment/src/main/java/io/quarkus/deployment/ExtensionLoader.java
Outdated
Show resolved
Hide resolved
core/deployment/src/main/java/io/quarkus/deployment/ExtensionLoader.java
Outdated
Show resolved
Hide resolved
...ent/src/main/java/io/quarkus/deployment/builditem/BootstrapConfigSetupCompleteBuildItem.java
Outdated
Show resolved
Hide resolved
@@ -6,13 +6,23 @@ | |||
public final class MainBytecodeRecorderBuildItem extends MultiBuildItem { | |||
|
|||
private final BytecodeRecorderImpl bytecodeRecorder; | |||
private final String generatedStartupContextClassName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If only one of these is ever set, should it be two different build items?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I did it this way is because the MainClassBuildStep
can continue to just consume List<MainBytecodeRecorderBuildItem>
and the order of generated StartupTask
objects is properly worked out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not loving this, but the Monday brain fails to come up with any better ideas, so... I guess it's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here :)
core/deployment/src/main/java/io/quarkus/deployment/steps/MainClassBuildStep.java
Outdated
Show resolved
Hide resolved
b0a65f5
to
ed2a377
Compare
@dmlloyd I think your concerns should now be addressed |
@dmlloyd is there anything else you think needs to be changed for this one? |
.../src/main/java/io/quarkus/deployment/builditem/RunTimeConfigurationSourceValueBuildItem.java
Outdated
Show resolved
Hide resolved
@dmlloyd suggestion applied |
...loyment/src/main/java/io/quarkus/deployment/configuration/RunTimeConfigurationGenerator.java
Outdated
Show resolved
Hide resolved
LOL, a suggested change caused the validator to fail, I never thought that would happen :P |
This round makes bootstrap config work properly with the build system without the arbitrary restrictions of the first iteration. This is accomplished mostly by moving config generation and setup into their own build steps
acf79fc
to
d31fe54
Compare
This implements the design that was discussed in smallrye/smallrye-config#215.
It introduces a new
Bootstrap
configuration phase that can be consumed by a Recorder whose intent is to produce aConfigSourceProvider
. These recorders are run during runtime init, but before the other Recorders. Their result contributes to the final runtimeConfig
creation.The part I am not too excited about is the integration into the build system since the new build items don't utilize the full power of the build system. However, 1) appropriate error messages are provided so people looking to create a bootstrap config implementation will be properly guided, 2) I don't see a good way of making it better (and avoiding hard-coding the specific Build Items) unless we introduce a new recording phase as well.