Introduce ApplicationConfig #1683

geoand · 2019-03-25T17:16:46Z

This allows users to set a desired name for the applications
(using quarkus.application.name) or version.
The idea came from:

#1300 (comment)

and this configuration is meant to be used by extensions that for some
reason need this information (like the planned kubernetes extension, where we need to inject something like ApplicationConfig in the processor)

Implementation notes:

I am not really sure if this is the simplest solution - it kind of seems like I went overboard...

The idea is to inject an ApplicationInfoBuildItem that has the default values, use that in ConfigurationSetup#initializeConfiguration to create a new ConfigSource with the default values.
This new source has a lower ordinal that the application.properties source and thus if values are set there (or in any of the sources with even higher ordinal), those values will be used.

If there is a simpler solution for creating the ApplicationConfig and populating it from with the values from the build, I'd love to hear it and implement it :)

dmlloyd · 2019-03-25T17:36:47Z

core/deployment/src/main/java/io/quarkus/deployment/steps/ConfigurationSetup.java

+            builder.withSources(
+                    inJar,
+                    // the ordinal of 240 because it's slightly lower priority then what inJar uses
+                    new PropertiesConfigSource(appStateProperties, "appState", 240));


This isn't the right way to set default values.

The best way to do it is to just use the defaultValue property on the @ConfigItem. The second-best way is to just implement whatever default behavior you want when the properties are not present in the config object. The third-best way would be to produce default value build items. Adding a config source doesn't even make the list unfortunately. ;-)

If you (or anyone) find yourself modifying the core configuration code or the core mojos or main methods in order to add a configuration switch, then a wrong turn was taken at some point. The configuration and build item infrastructures exist so that we don't have to do these things.

I really wanted to use defaultValue, but the problem is that default needs to be dynamic, in the sense that it depends on what is in the build.
My idea was that the "client" processors would just inject the ApplicationConfig and not have to worry about how to set default values if users don't.

So just to verify, what your proposing is that the processors would inject ApplicationConfig which would contain whatever the users set and then would also have to inject the default values in some other way?

The build step (singular) would accept the config and produce the build item with the actual app name in it. The build step would also calculate the default value for the build item in the event that it was not given in the config. Everything which needs the application name and/or version would consume the build item.

OK, got it thanks for the explanation!
So I implemented it the other way around. But I still don't understand how I would pull the default values (like the artifactId for the name) unless I am pushing them in when setting up the Augmentor.

The kubernetes extension would definitely need those values as well (by the means of injecting a build item that contains the final values).
I really like the idea of having these values settable in application.properties, but I think it would require a fair amount of work. WDYT?

As long as the Kube extension just consumed that build item, and assuming it was produced as I described above, it should just work with a minimum of effort.

Having the extension read the config should be easy once everything is in place. The difficulty I see is having all the config (like wiring classes) driven by application.properties

Ah I see. Well it doesn't make sense to force 100% of everything through properties. It's more like two polar endpoints: at one end, all application-wide config must be driven by application.properties, and at the other end, any per-class or per-member behavior should be driven by annotations. In between is where we consider factors to determine if it makes sense to make some configuration be driven one way and some the other, or whether to allow both.

No matter how it goes from the input stage, the next stage is to feed these into build items, so that consumers don't actually care where they came from.

👍 I will rework this PR starting tomorrow so we have something more concrete to comment on 😊

dmlloyd · 2019-03-25T17:41:03Z

core/devmode/src/main/java/io/quarkus/dev/DevModeMain.java

@@ -54,6 +58,9 @@ public static void main(String... args) throws Exception {
        classesRoot = new File(args[0]);
        wiringDir = new File(args[1]);
        cacheDir = new File(args[2]);
+        group = args[3];


I feel like we shouldn't be passing this in this way; the input should simply be the build configuration.

I agree that this is not cool, but I am not clear on how to pass the build configuration to the runtime runner

The "normal" approach would be to rely 100% on the configuration value and use a config root whose visibility is BUILD_AND_RUN_TIME_FIXED. However as discussed above, this wouldn't suffice in the case where no value was given and the default has to be computed.

So, we have to bring it in some other way. There are a few possibilities off the top of my head:

Produce a resource that contains the application name & version and load it at boot using a static init build step

Put it in a MANIFEST.MF property and load it at boot using a static init build step

Just generate a class with fields containing the name and version, hard-coded to the computed name and version

This is exactly the info I needed to close the loop, thank you! Now everything makes sense. I hadn't seen the any relevant parts of the code so I didn't know how to handle such cases, but with the explanation you just provided plus the information from the other comment I see the way forward.

I'll pick this up again tomorrow most likely and come up with proper solution that adheres to these guidelines.

Thanks for taking the time to explain!

dmlloyd · 2019-03-25T17:42:57Z

core/deployment/src/main/java/io/quarkus/deployment/QuarkusAugmentor.java

@@ -84,6 +85,11 @@ public BuildResult run() throws Exception {
                    .addInitial(LaunchModeBuildItem.class)
                    .addInitial(AdditionalApplicationArchiveBuildItem.class)
                    .addInitial(ExtensionClassLoaderBuildItem.class);
+
+            if (!applicationInfo.isUnset()) {
+                chainBuilder.addInitial(ApplicationInfoBuildItem.class);


This could just as well be produced from a build step which consumes the ApplicationConfig object.

dmlloyd · 2019-03-25T17:43:10Z

core/deployment/src/main/java/io/quarkus/deployment/QuarkusAugmentor.java

@@ -103,6 +109,11 @@ public BuildResult run() throws Exception {
                    .produce(new ShutdownContextBuildItem())
                    .produce(new LaunchModeBuildItem(launchMode))
                    .produce(new ExtensionClassLoaderBuildItem(classLoader));
+
+            if (!applicationInfo.isUnset()) {
+                execBuilder.produce(new ApplicationInfoBuildItem(applicationInfo));


geoand · 2019-03-26T11:26:33Z

@dmlloyd I pushed a completely revised version. It should be simpler and move inline with the internal Quarkus philosophy ( I hope :) ). The names are probably bad, since I am pretty terrible at naming...
Comments obviously more than welcome.

dmlloyd

The passing of values through a properties file isn't quite what I had in mind, but it's OK because we don't really have anything better at the moment. I'll take on an issue to improve that.

dmlloyd · 2019-03-26T12:29:40Z

Pending CI.

geoand · 2019-03-26T12:31:29Z

@dmlloyd Just out of curiosity, what do you have in mind as far as improvements go?
Also FYI, I'm probably going to need to add more properties (coming from the build environment like wiring-classes) for the kubernetes extension.

dmlloyd · 2019-03-26T12:35:03Z

I've opened #1693 with more info on that.

geoand · 2019-03-26T12:35:39Z

Cool

geoand · 2019-03-26T12:46:43Z

The native test seems to be failing with:

Error: Class initialization failed: io.quarkus.runtime.generated.BuildTimeConfig
Original exception that caused the problem: java.lang.NoClassDefFoundError: io/quarkus/deployment/ApplicationConfig$$accessor

in HibernateValidatorFunctionalityTest.

This seems pretty weird... Is there anything that you see that I have missed @dmlloyd ?

dmlloyd · 2019-03-26T12:49:20Z

Yes, I see the bug. The config class is in the deployment module but it has a run time retention.

Since you're using build items anyway, the config scope for that ConfigRoot should just be BUILD_TIME.

This allows users to set a desired name for the applications (using quarkus.application.name) or version. The idea came from: quarkusio#1300 (comment) and this configuration is meant to be used by extensions that for some reason need this information (like the planned kubernetes extension)

geoand · 2019-03-26T12:50:19Z

Thanks! PR updated

geoand requested a review from dmlloyd March 25, 2019 17:16

dmlloyd requested changes Mar 25, 2019

View reviewed changes

geoand added the coding-in-progress label Mar 25, 2019

geoand force-pushed the appconfig branch from 187f875 to d7d9f3a Compare March 26, 2019 11:24

geoand requested a review from dmlloyd March 26, 2019 11:28

geoand force-pushed the appconfig branch 2 times, most recently from a687249 to 81d61b6 Compare March 26, 2019 12:20

dmlloyd approved these changes Mar 26, 2019

View reviewed changes

geoand force-pushed the appconfig branch from 81d61b6 to 4cb953c Compare March 26, 2019 12:50

geoand merged commit 4083a3c into quarkusio:master Mar 26, 2019

geoand deleted the appconfig branch March 26, 2019 14:04

geoand removed the coding-in-progress label Mar 26, 2019

geoand mentioned this pull request Mar 26, 2019

Make sure ApplicationConfig uses the correct resource dir #1695

Closed

gsmet added this to the 0.13.0 milestone Apr 2, 2019

jorsol mentioned this pull request Jun 29, 2019

No config value of type [java.lang.String] exists for: quarkus.application.name #3008

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce ApplicationConfig #1683

Introduce ApplicationConfig #1683

geoand commented Mar 25, 2019 •

edited

Loading

dmlloyd Mar 25, 2019

dmlloyd Mar 25, 2019

geoand Mar 25, 2019

dmlloyd Mar 25, 2019

geoand Mar 25, 2019 •

edited

Loading

geoand Mar 25, 2019

dmlloyd Mar 25, 2019

geoand Mar 25, 2019

dmlloyd Mar 25, 2019

geoand Mar 25, 2019

dmlloyd Mar 25, 2019

geoand Mar 25, 2019

dmlloyd Mar 25, 2019

geoand Mar 25, 2019

dmlloyd Mar 25, 2019

dmlloyd Mar 25, 2019

geoand commented Mar 26, 2019 •

edited

Loading

dmlloyd left a comment

dmlloyd commented Mar 26, 2019

geoand commented Mar 26, 2019 •

edited

Loading

dmlloyd commented Mar 26, 2019

geoand commented Mar 26, 2019

geoand commented Mar 26, 2019

dmlloyd commented Mar 26, 2019

geoand commented Mar 26, 2019

Introduce ApplicationConfig #1683

Introduce ApplicationConfig #1683

Conversation

geoand commented Mar 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoand Mar 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoand commented Mar 26, 2019 • edited Loading

dmlloyd left a comment

Choose a reason for hiding this comment

dmlloyd commented Mar 26, 2019

geoand commented Mar 26, 2019 • edited Loading

dmlloyd commented Mar 26, 2019

geoand commented Mar 26, 2019

geoand commented Mar 26, 2019

dmlloyd commented Mar 26, 2019

geoand commented Mar 26, 2019

geoand commented Mar 25, 2019 •

edited

Loading

geoand Mar 25, 2019 •

edited

Loading

geoand commented Mar 26, 2019 •

edited

Loading

geoand commented Mar 26, 2019 •

edited

Loading