You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kie.apache.org by "gitgabrio (via GitHub)" <gi...@apache.org> on 2026/04/20 12:37:54 UTC
[PR] [incubator-kie-issues#2164] Implement property retrieval from YAML files [incubator-kie-kogito-runtimes]
gitgabrio opened a new pull request, #4260:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/4260
Fixes https://github.com/apache/incubator-kie-issues/issues/2164
Needed by:
This PR:
1. modify property retrieval for quarkus environment
2. add properties retrieved from yaml files to the one discovered by previous approach
<details>
<summary>
How to replicate CI configuration locally?
</summary>
Build Chain tool does "simple" maven build(s), the builds are just Maven commands, but because the repositories relates and depends on each other and any change in API or class method could affect several of those repositories there is a need to use [build-chain tool](https://github.com/kiegroup/github-action-build-chain) to handle cross repository builds and be sure that we always use latest version of the code for each repository.
[build-chain tool](https://github.com/kiegroup/github-action-build-chain) is a build tool which can be used on command line locally or in Github Actions workflow(s), in case you need to change multiple repositories and send multiple dependent pull requests related with a change you can easily reproduce the same build by executing it on Github hosted environment or locally in your development environment. See [local execution](https://github.com/kiegroup/github-action-build-chain#local-execution) details to get more information about it.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#2164] Implement property retrieval from YAML files [incubator-kie-kogito-runtimes]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #4260:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/4260#discussion_r3117461265
##########
quarkus/extensions/kogito-quarkus-extension-common/kogito-quarkus-common-deployment/src/main/java/org/kie/kogito/quarkus/common/deployment/KogitoQuarkusApplicationPropertiesProvider.java:
##########
@@ -18,39 +18,73 @@
*/
package org.kie.kogito.quarkus.common.deployment;
+import java.io.File;
+import java.net.URL;
import java.util.Collection;
import java.util.Optional;
-import java.util.stream.StreamSupport;
+import java.util.Properties;
+import org.eclipse.microprofile.config.Config;
import org.eclipse.microprofile.config.ConfigProvider;
import org.kie.kogito.codegen.api.context.KogitoApplicationPropertyProvider;
-import static java.util.stream.Collectors.toSet;
+import static org.drools.codegen.common.DroolsModelBuildContext.APPLICATION_PROPERTIES_YAML_FILE_NAME;
+import static org.drools.codegen.common.DroolsModelBuildContext.APPLICATION_PROPERTIES_YML_FILE_NAME;
+import static org.kie.kogito.codegen.api.context.impl.AbstractKogitoBuildContext.loadYmlProperties;
+import static org.kie.kogito.internal.utils.ConversionUtils.convert;
public class KogitoQuarkusApplicationPropertiesProvider implements KogitoApplicationPropertyProvider {
+ private final Properties properties;
+
+ public KogitoQuarkusApplicationPropertiesProvider() {
+ this.properties = new Properties();
+ Config config = ConfigProvider.getConfig();
+ config.getPropertyNames().forEach(property -> {
+ Optional<String> storedProperty = config.getOptionalValue(property, String.class);
+ if (storedProperty.isPresent()) {
+ this.properties.put(property, storedProperty.get());
+ } else {
+ this.properties.put(property, "");
+ }
+ });
+ URL ymlResource = Thread.currentThread().getContextClassLoader().getResource(APPLICATION_PROPERTIES_YML_FILE_NAME);
+ if (ymlResource != null) {
+ File ymlFile = new File(ymlResource.getFile());
+ loadYmlProperties(ymlFile, properties);
+ }
+ URL yamlResource = Thread.currentThread().getContextClassLoader().getResource(APPLICATION_PROPERTIES_YAML_FILE_NAME);
+ if (yamlResource != null) {
+ File yamlFile = new File(yamlResource.getFile());
+ loadYmlProperties(yamlFile, properties);
+ }
+
+ }
+
@Override
public Optional<String> getApplicationProperty(String property) {
- return ConfigProvider.getConfig().getOptionalValue(property, String.class);
+ return Optional.ofNullable(properties.getProperty(property));
}
@Override
public Collection<String> getApplicationProperties() {
- return StreamSupport.stream(ConfigProvider.getConfig().getPropertyNames().spliterator(), false).collect(toSet());
+ return properties.stringPropertyNames();
}
@Override
public <T> Optional<T> getApplicationProperty(String property, Class<T> clazz) {
- return ConfigProvider.getConfig().getOptionalValue(property, clazz);
+ return Optional.ofNullable(convert(properties.getProperty(property), clazz));
Review Comment:
This new YAML merge/caching behavior in the Quarkus property provider isn’t covered by tests in this module. Adding a unit test that places an `application.yml`/`application.yaml` on the test classpath and asserts (1) keys are discovered and (2) precedence vs MP Config values is correct would help prevent regressions.
##########
quarkus/extensions/kogito-quarkus-extension-common/kogito-quarkus-common-deployment/src/main/java/org/kie/kogito/quarkus/common/deployment/KogitoQuarkusApplicationPropertiesProvider.java:
##########
@@ -18,39 +18,73 @@
*/
package org.kie.kogito.quarkus.common.deployment;
+import java.io.File;
+import java.net.URL;
import java.util.Collection;
import java.util.Optional;
-import java.util.stream.StreamSupport;
+import java.util.Properties;
+import org.eclipse.microprofile.config.Config;
import org.eclipse.microprofile.config.ConfigProvider;
import org.kie.kogito.codegen.api.context.KogitoApplicationPropertyProvider;
-import static java.util.stream.Collectors.toSet;
+import static org.drools.codegen.common.DroolsModelBuildContext.APPLICATION_PROPERTIES_YAML_FILE_NAME;
+import static org.drools.codegen.common.DroolsModelBuildContext.APPLICATION_PROPERTIES_YML_FILE_NAME;
+import static org.kie.kogito.codegen.api.context.impl.AbstractKogitoBuildContext.loadYmlProperties;
+import static org.kie.kogito.internal.utils.ConversionUtils.convert;
public class KogitoQuarkusApplicationPropertiesProvider implements KogitoApplicationPropertyProvider {
+ private final Properties properties;
+
+ public KogitoQuarkusApplicationPropertiesProvider() {
+ this.properties = new Properties();
+ Config config = ConfigProvider.getConfig();
+ config.getPropertyNames().forEach(property -> {
+ Optional<String> storedProperty = config.getOptionalValue(property, String.class);
+ if (storedProperty.isPresent()) {
+ this.properties.put(property, storedProperty.get());
+ } else {
+ this.properties.put(property, "");
+ }
+ });
+ URL ymlResource = Thread.currentThread().getContextClassLoader().getResource(APPLICATION_PROPERTIES_YML_FILE_NAME);
+ if (ymlResource != null) {
+ File ymlFile = new File(ymlResource.getFile());
+ loadYmlProperties(ymlFile, properties);
+ }
+ URL yamlResource = Thread.currentThread().getContextClassLoader().getResource(APPLICATION_PROPERTIES_YAML_FILE_NAME);
+ if (yamlResource != null) {
+ File yamlFile = new File(yamlResource.getFile());
+ loadYmlProperties(yamlFile, properties);
+ }
+
+ }
+
Review Comment:
YAML properties are loaded *after* all values from MicroProfile Config have been cached, so `loadYmlProperties(..., properties)` can override values coming from higher-precedence sources (system properties, env vars, etc.). The PR description says YAML values should be added to the previously discovered properties; to preserve Config precedence, consider only inserting YAML keys that are not already present in `properties` (or load YAML first and let Config override).
```suggestion
loadYmlPropertiesIfAbsent(ymlFile);
}
URL yamlResource = Thread.currentThread().getContextClassLoader().getResource(APPLICATION_PROPERTIES_YAML_FILE_NAME);
if (yamlResource != null) {
File yamlFile = new File(yamlResource.getFile());
loadYmlPropertiesIfAbsent(yamlFile);
}
}
private void loadYmlPropertiesIfAbsent(File ymlFile) {
Properties yamlProperties = new Properties();
loadYmlProperties(ymlFile, yamlProperties);
yamlProperties.forEach((key, value) -> this.properties.putIfAbsent(key, value));
}
```
##########
quarkus/extensions/kogito-quarkus-extension-common/kogito-quarkus-common-deployment/src/main/java/org/kie/kogito/quarkus/common/deployment/KogitoQuarkusApplicationPropertiesProvider.java:
##########
@@ -18,39 +18,73 @@
*/
package org.kie.kogito.quarkus.common.deployment;
+import java.io.File;
+import java.net.URL;
import java.util.Collection;
import java.util.Optional;
-import java.util.stream.StreamSupport;
+import java.util.Properties;
+import org.eclipse.microprofile.config.Config;
import org.eclipse.microprofile.config.ConfigProvider;
import org.kie.kogito.codegen.api.context.KogitoApplicationPropertyProvider;
-import static java.util.stream.Collectors.toSet;
+import static org.drools.codegen.common.DroolsModelBuildContext.APPLICATION_PROPERTIES_YAML_FILE_NAME;
+import static org.drools.codegen.common.DroolsModelBuildContext.APPLICATION_PROPERTIES_YML_FILE_NAME;
+import static org.kie.kogito.codegen.api.context.impl.AbstractKogitoBuildContext.loadYmlProperties;
+import static org.kie.kogito.internal.utils.ConversionUtils.convert;
public class KogitoQuarkusApplicationPropertiesProvider implements KogitoApplicationPropertyProvider {
+ private final Properties properties;
+
+ public KogitoQuarkusApplicationPropertiesProvider() {
+ this.properties = new Properties();
+ Config config = ConfigProvider.getConfig();
+ config.getPropertyNames().forEach(property -> {
+ Optional<String> storedProperty = config.getOptionalValue(property, String.class);
+ if (storedProperty.isPresent()) {
+ this.properties.put(property, storedProperty.get());
+ } else {
+ this.properties.put(property, "");
+ }
+ });
+ URL ymlResource = Thread.currentThread().getContextClassLoader().getResource(APPLICATION_PROPERTIES_YML_FILE_NAME);
+ if (ymlResource != null) {
+ File ymlFile = new File(ymlResource.getFile());
+ loadYmlProperties(ymlFile, properties);
+ }
+ URL yamlResource = Thread.currentThread().getContextClassLoader().getResource(APPLICATION_PROPERTIES_YAML_FILE_NAME);
+ if (yamlResource != null) {
+ File yamlFile = new File(yamlResource.getFile());
+ loadYmlProperties(yamlFile, properties);
+ }
Review Comment:
`new File(ymlResource.getFile())` / `new File(yamlResource.getFile())` is fragile: `URL#getFile()` returns URL-encoded paths (e.g., spaces as `%20`) and will not work for non-`file:` resources (e.g., `jar:`). This can make YAML loading fail depending on how the build is launched. Prefer resolving the URL via `toURI()`/`Paths.get(...)` for `file:` URLs, and consider adding an overload of `loadYmlProperties` that accepts an `InputStream` so resources can be read directly when not backed by a filesystem file.
##########
quarkus/extensions/kogito-quarkus-extension-common/kogito-quarkus-common-deployment/src/main/java/org/kie/kogito/quarkus/common/deployment/KogitoQuarkusApplicationPropertiesProvider.java:
##########
@@ -18,39 +18,73 @@
*/
package org.kie.kogito.quarkus.common.deployment;
+import java.io.File;
+import java.net.URL;
import java.util.Collection;
import java.util.Optional;
-import java.util.stream.StreamSupport;
+import java.util.Properties;
+import org.eclipse.microprofile.config.Config;
import org.eclipse.microprofile.config.ConfigProvider;
import org.kie.kogito.codegen.api.context.KogitoApplicationPropertyProvider;
-import static java.util.stream.Collectors.toSet;
+import static org.drools.codegen.common.DroolsModelBuildContext.APPLICATION_PROPERTIES_YAML_FILE_NAME;
+import static org.drools.codegen.common.DroolsModelBuildContext.APPLICATION_PROPERTIES_YML_FILE_NAME;
+import static org.kie.kogito.codegen.api.context.impl.AbstractKogitoBuildContext.loadYmlProperties;
+import static org.kie.kogito.internal.utils.ConversionUtils.convert;
public class KogitoQuarkusApplicationPropertiesProvider implements KogitoApplicationPropertyProvider {
+ private final Properties properties;
+
+ public KogitoQuarkusApplicationPropertiesProvider() {
+ this.properties = new Properties();
+ Config config = ConfigProvider.getConfig();
+ config.getPropertyNames().forEach(property -> {
+ Optional<String> storedProperty = config.getOptionalValue(property, String.class);
+ if (storedProperty.isPresent()) {
+ this.properties.put(property, storedProperty.get());
+ } else {
+ this.properties.put(property, "");
Review Comment:
The constructor stores an empty string when `Config#getOptionalValue(..., String.class)` returns empty. That changes the semantics of `getApplicationProperty(...)` from `Optional.empty()` to `Optional.of("")` and can also cause type conversions to fail (e.g., parsing "" as an integer/long) for callers using `getApplicationProperty(key, clazz)`. Prefer skipping the entry when the Optional is empty (or only caching keys with present values).
```suggestion
```
##########
kogito-codegen-modules/kogito-codegen-api/src/main/java/org/kie/kogito/codegen/api/context/impl/AbstractKogitoBuildContext.java:
##########
@@ -112,7 +112,7 @@ protected static Properties load(File... resourcePaths) {
return applicationProperties;
}
- protected static void loadYmlProperties(File ymlFile, Properties applicationProperties) {
+ public static void loadYmlProperties(File ymlFile, Properties applicationProperties) {
Map<String, String> ymlMap = loadYmlStringMap(ymlFile);
Review Comment:
Changing `loadYmlProperties` from `protected` to `public` exposes an internal helper on `AbstractKogitoBuildContext` as part of the `kogito-codegen-api` artifact. If this is only needed by external callers (like the Quarkus deployment module), consider moving YAML-loading to a dedicated utility class in the API (or another appropriate module) to avoid expanding the build-context API surface and coupling callers to the `impl` package.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-issues#2164] Implement property retrieval from YAML files [incubator-kie-kogito-runtimes]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio merged PR #4260:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/4260
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org