Skip to content

CAMEL-23276: Add PluginRunCustomizer to JBang Plugin SPI#22473

Merged
Croway merged 3 commits intoapache:mainfrom
Croway:CAMEL-23276-plugin-run-customizer
Apr 8, 2026
Merged

CAMEL-23276: Add PluginRunCustomizer to JBang Plugin SPI#22473
Croway merged 3 commits intoapache:mainfrom
Croway:CAMEL-23276-plugin-run-customizer

Conversation

@Croway
Copy link
Copy Markdown
Contributor

@Croway Croway commented Apr 7, 2026

Summary

  • Adds PluginRunCustomizer interface with beforeRun(KameletMain, List<String>) hook
  • Adds Plugin.getRunCustomizer() default method (follows getExporter() pattern)
  • Run.run() invokes active plugin customizers after dependency resolution but before KameletMain.start()
  • Respects --skip-plugins flag

This 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 arguments

Claude Code on behalf of Federico Mariani

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 7, 2026

🌟 Thank you for your contribution to the Apache Camel project! 🌟
🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

  • First-time contributors require MANUAL approval for the GitHub Actions to run
  • You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot although they are normally detected and executed by CI.
  • You can label PRs using build-all, build-dependents, skip-tests and test-dependents to fine-tune the checks executed by this PR.
  • Build and test logs are available in the summary page. Only Apache Camel committers have access to the summary.

⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

@github-actions github-actions bot added the dsl label Apr 7, 2026
Move PluginRunCustomizer.beforeRun() to run before
PluginExporter.getDependencies() so plugins can set up
config directories before dependency scanning occurs.
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Apr 7, 2026

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Copy link
Copy Markdown
Contributor

@gnodet gnodet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Javadoc
  • Plugin.java:47-49getRunCustomizer() Javadoc
  • Run.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 beforeRun Javadoc

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 on Plugin — consistent SPI style.
  • The --skip-plugins flag is respected.
  • Tests cover both the default (empty) and custom implementation paths.

Claude Code on behalf of Guillaume Nodet

@Croway
Copy link
Copy Markdown
Contributor Author

Croway commented Apr 8, 2026

thanks for the review @gnodet , I've implemented the medium findings in the latest commit

@Croway Croway force-pushed the CAMEL-23276-plugin-run-customizer branch from a21de89 to aa89134 Compare April 8, 2026 09:07
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

🧪 CI tested the following changed modules:

  • dsl/camel-jbang/camel-jbang-core

⚠️ Some tests are disabled on GitHub Actions (@DisabledIfSystemProperty(named = "ci.env.name")) and require manual verification:

  • dsl/camel-jbang/camel-jbang-core: 1 test(s) disabled on GitHub Actions

💡 Manual integration tests recommended:

You modified dsl/camel-jbang/camel-jbang-core. The related integration tests in dsl/camel-jbang/camel-jbang-it are excluded from CI. Consider running them manually:

mvn verify -f dsl/camel-jbang/camel-jbang-it -Djbang-it-test

⚙️ View full build and test results

@Croway
Copy link
Copy Markdown
Contributor Author

Croway commented Apr 8, 2026

The CI failures are unrelated

@Croway Croway merged commit 1df033f into apache:main Apr 8, 2026
3 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants