CAMEL-23276: Add PluginRunCustomizer to JBang Plugin SPI#22473
CAMEL-23276: Add PluginRunCustomizer to JBang Plugin SPI#22473Croway merged 3 commits intoapache:mainfrom
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
Move PluginRunCustomizer.beforeRun() to run before PluginExporter.getDependencies() so plugins can set up config directories before dependency scanning occurs.
|
gnodet
left a comment
There was a problem hiding this comment.
Review Findings
Medium
1. Javadoc contradicts the actual insertion point
The Javadoc on PluginRunCustomizer and Plugin.getRunCustomizer() both state:
"called after the Run command has resolved file arguments and configured dependencies"
But the inline comment in Run.java says the opposite:
"before dependency resolution, so plugin exporters can scan the right locations"
Looking at the actual code position, the customizer runs after basic dependency resolution (addDependencies, addRuntimeSpecificDependenciesFromProperties) but before plugin exporter dependencies are added. The inline comment is closer to the truth — the Javadoc is misleading. One or the other should be corrected so implementors know the exact lifecycle point.
PluginRunCustomizer.java:25-28— interface-level JavadocPlugin.java:47-49—getRunCustomizer()JavadocRun.java:971-972— inline comment
2. Mutable files list passed to plugins
files is a public ArrayList<String> field on the Run command (line 158). It is passed directly to beforeRun(main, files). A misbehaving plugin could mutate this list (add/remove/clear), affecting all subsequent processing in Run.run(). Consider either:
- Passing
Collections.unmodifiableList(files), or - Documenting the contract (read-only vs. mutable) in the
beforeRunJavadoc
Low / Style
3. Variable scope
activePlugins is declared outside the if (!skipPlugins) block and initialized to Collections.emptyMap(), but is only used inside the block. It could be declared inside, removing the unnecessary initialization.
4. Missing JIRA link in PR body
Per project conventions, the PR body should include a clickable link to CAMEL-23276.
Questions
5. Future extensibility
The beforeRun hook receives KameletMain and files. If plugins later need access to the resolved dependencies or profileProperties, the signature would need to change (breaking the SPI). Is the current parameter set sufficient for the intended use cases, or should a context object be considered?
What looks good
- The design follows the established
getExporter()pattern onPlugin— consistent SPI style. - The
--skip-pluginsflag is respected. - Tests cover both the default (empty) and custom implementation paths.
Claude Code on behalf of Guillaume Nodet
|
thanks for the review @gnodet , I've implemented the medium findings in the latest commit |
a21de89 to
aa89134
Compare
|
🧪 CI tested the following changed modules:
💡 Manual integration tests recommended:
|
|
The CI failures are unrelated |


Summary
PluginRunCustomizerinterface withbeforeRun(KameletMain, List<String>)hookPlugin.getRunCustomizer()default method (followsgetExporter()pattern)Run.run()invokes active plugin customizers after dependency resolution but beforeKameletMain.start()--skip-pluginsflagThis allows third-party plugins to customize the environment (system properties, config directories, initial properties) based on file arguments, without needing to subclass the Run command.
Test plan
PluginRunCustomizerTest: verifies default returns empty, verifies customizer receives correct argumentsClaude Code on behalf of Federico Mariani