gnodet-bot commented on code in PR #29578:
URL: https://github.com/apache/camel/pull/29578#discussion_r4530463687
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/common/PluginHelper.java:
##########
@@ -86,6 +88,37 @@ private PluginHelper() {
// prevent instantiation of utility class
}
+ public static Path getCustomPluginConfigPath() {
+ return getCustomPluginConfigPath(null);
+ }
+
+ /**
+ * Resolves the path to the plugin configuration file. Checks, in order:
+ * <ol>
+ * <li>A path set programmatically via method argument</li>
+ * <li>The {@value #PLUGIN_CONFIG_ENV} environment variable</li>
+ * <li>The default {@value #PLUGIN_CONFIG_FILE} in the user home
directory</li>
+ * </ol>
+ */
+ public static Path getCustomPluginConfigPath(Path configPath) {
+ Path pluginConfigPath;
+ if (configPath != null) {
+ pluginConfigPath = configPath;
+ } else {
+ String env = System.getenv(PLUGIN_CONFIG_ENV);
+ if (env != null && !env.isBlank()) {
+ pluginConfigPath = Path.of(env);
Review Comment:
⚠️ **`Path.of(env)` can throw `InvalidPathException`** — this is an
unchecked exception (`extends IllegalArgumentException`) that will propagate up
and crash the CLI with a confusing stack trace when `CAMEL_CLI_PLUGIN_CONFIG`
contains illegal path characters (e.g. null bytes on Linux, `<>:|?*` on
Windows).
Wrap with a try-catch and emit a user-friendly error:
```suggestion
try {
pluginConfigPath = Path.of(env);
} catch (java.nio.file.InvalidPathException e) {
throw new IllegalStateException(
"Invalid path in " + PLUGIN_CONFIG_ENV + "
environment variable: " + env, e);
}
```
##########
dsl/camel-jbang/camel-jbang-core/src/test/java/org/apache/camel/dsl/jbang/core/common/PluginHelperTest.java:
##########
@@ -395,12 +396,126 @@ void testNoCompatibilityWarningForLegacyEntry() {
assertEquals("", printer.getOutput());
}
+ @Test
+ public void testDefaultPluginConfigPath() {
+ Path expected =
CommandLineHelper.getHomeDir().resolve(PluginHelper.PLUGIN_CONFIG_FILE);
+ assertEquals(expected, PluginHelper.getCustomPluginConfigPath());
+ }
+
+ @Test
+ public void testCustomPluginConfigPath() {
+ Path custom = tempDir.resolve("custom-plugins.json");
+ assertEquals(custom, PluginHelper.getCustomPluginConfigPath(custom));
+ }
+
+ @Test
+ public void testCustomPluginConfigPathAsDirectory() throws Exception {
+ Path customDir = tempDir.resolve("custom-config-dir");
+ Files.createDirectories(customDir);
+ assertEquals(customDir.resolve(PluginHelper.PLUGIN_CONFIG_FILE),
PluginHelper.getCustomPluginConfigPath(customDir));
+ }
+
+ @Test
+ public void testCustomPluginConfigPathReset() {
Review Comment:
⚠️ **Missing test for `CAMEL_CLI_PLUGIN_CONFIG` env var** — the diff adds 8
test methods for the explicit-path scenarios but none exercises the
`System.getenv(PLUGIN_CONFIG_ENV)` branch. This is a documented first-class
feature with zero coverage.
`junit-pioneer` is already a test-scope dependency in
`camel-jbang-core/pom.xml`, so `@SetEnvironmentVariable` is available without
adding new deps:
```java
@Test
@SetEnvironmentVariable(key = PluginHelper.PLUGIN_CONFIG_ENV, value =
"/tmp/camel-test-plugins.json")
public void testEnvVarPluginConfigPath() {
assertEquals(Path.of("/tmp/camel-test-plugins.json"),
PluginHelper.getCustomPluginConfigPath());
}
@Test
@SetEnvironmentVariable(key = PluginHelper.PLUGIN_CONFIG_ENV, value =
"/tmp/camel-config-dir")
public void testEnvVarPluginConfigPathDirectory() throws Exception {
Files.createDirectories(Path.of("/tmp/camel-config-dir"));
assertEquals(
Path.of("/tmp/camel-config-dir").resolve(PluginHelper.PLUGIN_CONFIG_FILE),
PluginHelper.getCustomPluginConfigPath()
);
}
```
Also add a test verifying that an explicit `configPath` argument takes
precedence over the env var (the override documented in the PR description).
Note: for `@SetEnvironmentVariable` to work, the
`jvm-open-language-management` module arg is required in
`maven-surefire-plugin` configuration — check how
`camel-jbang-plugin-kubernetes` sets it up (`--add-opens
java.base/java.util=ALL-UNNAMED`).
_Import needed: `import org.junitpioneer.jupiter.SetEnvironmentVariable;`_
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
