CAMEL-24226: camel run/export - add --resource-dirs option - #24986
Conversation
…rectories preserving structure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
There was a problem hiding this comment.
Review: CAMEL-24226 — camel run/export --resource-dirs option
Well-crafted feature with clean code, thorough documentation (all command reference pages updated), and good test coverage (9 tests). The security safeguards — absolute path rejection, 1000-file limit, try-with-resources on Files.walk — are solid.
Findings
1. copyResourceDirs edge case with bare --resource-dirs=..
When the user passes --resource-dirs=.., Path.of("..").getFileName() returns "..", so target = srcResourcesDir.resolve(baseName) resolves to src/main/resources/.. → src/main/, writing files one level above the intended target. This isn't a security vulnerability (CLI user is trusted and has filesystem access), but it's a correctness issue — files silently end up in the wrong location.
A guard in validateResourceDir rejecting paths whose getFileName() is ".." would close it:
if (path.getFileName() != null && path.getFileName().toString().equals("..")) {
throw new IllegalArgumentException(
"--resource-dirs path must end with a named directory, not '..': " + dir);
}The existing test nestedParentPathShouldMapToLastComponent covers ../../mydata/foo (named last component ✅) but not the bare .. case.
2. Missing upgrade guide entry
New CLI option — per project convention, a brief mention in camel-4x-upgrade-guide-4_22.adoc would help discoverability.
Neither blocks the merge — the edge case requires a user to specifically pass .. as the sole resource dir, and the upgrade guide is a convention item.
Reviewed with Claude Code on behalf of gnodet. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
|
Thanks for the review. Added the guard in The upgrade guide entry — I'll skip for now as this is a new option (not a change to existing behavior). |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
gnodet
left a comment
There was a problem hiding this comment.
Re-review: CAMEL-24226 — camel run/export --resource-dirs option
Re-reviewing after the latest commit. The bare .. edge case I flagged has been properly addressed:
if (path.getFileName() != null && path.getFileName().toString().equals("..")) {
throw new IllegalArgumentException(
"--resource-dirs path must end with a named directory, not '..': " + dir);
}And the validateShouldRejectBareDotDot test covers it. Good fix. ✅
The code is clean — validation, recursive walking, Kubernetes integration all look correct. Documentation is thorough.
CI note
CI is currently failing with "Fail if there are uncommitted changes" — generated files need to be regenerated and committed.
Still open (non-blocking)
Upgrade guide entry for the new CLI option (camel-4x-upgrade-guide-4_22.adoc).
Will approve once CI is green.
Reviewed with Claude Code on behalf of gnodet. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
gnodet
left a comment
There was a problem hiding this comment.
Claude Code on behalf of gnodet
✅ LGTM — CI fix applied, promoting to approve
Re-reviewing after the new commit chore: regen jbang configuration metadata for resourceDirs option which addresses the CI failure I flagged in my previous review (uncommitted generated files).
Java source code — unchanged from my previous review. The validateResourceDir() bare .. rejection and the rest of the implementation remain sound.
Generated files — jbang configuration metadata now properly committed, which should resolve the CI "uncommitted changes" check.
Documentation — good coverage in both camel-jbang-running.adoc and camel-jbang-projects.adoc with examples.
My earlier suggestion about an upgrade guide entry for the new --resource-dirs option still stands — it helps users discover new features when upgrading — but it's not blocking.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 12 tested, 23 compile-only — current: 8 all testedMaveniverse Scalpel detected 35 affected modules (current approach: 8).
|
Summary
--resource-dirsoption tocamel run,camel export, andcamel kubernetes run/exportcommandssrc/main/resourcespreserving the full directory structurecamel run, adds files to classpath with their directory-relative paths preservedcamel export, copies files intosrc/main/resources/<dir>/...preserving structure..(e.g.,../shared/schemas), uses only the last directory name as target (schemas/)Example:
With
soap/schemas/common.xsdproducessrc/main/resources/soap/schemas/common.xsd.Test plan
ResourceDirsTestwith 9 tests covering: validation, recursive walking, max file limit,..path mapping, deep subdirectory structureClaude Code on behalf of davsclaus