christophd commented on PR #29578: URL: https://github.com/apache/camel/pull/29578#issuecomment-6791785873
_Claude Code on behalf of christophd_ Thanks for the thorough review! All points addressed in 8ce02ad: **Blocking** 1. **CI failures fixed.** `PluginCommand` now extends `CamelCommand` directly instead of `PluginBaseCommand`, so `camel plugin` no longer inherits `--plugin-config`. The usage line is back to `Usage: camel plugin [-h] [COMMAND]`, which fixes both `CliHelpGoldenTest` (golden file regenerated) and `ParameterExceptionHandlerTest` (test passes unchanged). 2. **`--plugin-config` removed from the parent.** Since `PluginCommand` extends `CamelCommand` directly, `camel plugin --plugin-config X` is no longer accepted. Users write `camel plugin get --plugin-config X`, `camel plugin add --plugin-config X`, etc. — the option only exists on the subcommands that actually call `loadConfig`/`saveConfig`. **Important** 3. **Documentation added.** New "Custom plugin configuration" section in `camel-jbang-projects.adoc` documents `CAMEL_CLI_PLUGIN_CONFIG`, the precedence order, file vs directory, and the important scope limitation that `--plugin-config` only applies to `plugin` subcommands. The option description in `PluginBaseCommand` was updated to mention this too. 4. **Env var tests added.** Extracted a package-private `resolvePluginConfigPath(Path configPath, String envValue)` that takes the env value as a parameter — avoids needing `@SetEnvironmentVariable` and `--add-opens`. Four new tests: env var as file, env var as directory, explicit path takes precedence over env var, and invalid env var path throws `IllegalStateException`. **Minor and nits** 5. **File vs directory detection improved.** Now checks `Files.isDirectory(path)` first, then uses a case-insensitive `.json` suffix with null-safe `getFileName()` for paths that don't exist yet. Handles root paths, `PLUGINS.JSON`, and existing non-`.json` files correctly. 6. **`InvalidPathException` handling added.** Both `PluginHelper.resolvePluginConfigPath()` and `PluginBaseCommand.pluginConfigPath()` catch `InvalidPathException` and throw `IllegalStateException` with a message naming the source (`CAMEL_CLI_PLUGIN_CONFIG` or `--plugin-config`). 7. **`putIfAbsent`:** This is intentional — when a built-in plugin is installed on first use (e.g. the first `camel kubernetes` invocation), `enable()` should preserve any existing entry that the user previously configured with `camel plugin add` (which may have a pinned version, custom repos, or a `resolved` block from the cache). Using `put` would overwrite those user-set values with the bare defaults. 8. **Constant rename reverted.** `PLUGIN_CONFIG_FILE` is back to `PLUGIN_CONFIG`. Duplicate test `testCustomPluginConfigPathReset` removed. -- 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]
