CAMEL-24236: camel-cyberark-vault - add support for cyberark policies - #25012
Conversation
d1ae255 to
6fc53ce
Compare
gnodet
left a comment
There was a problem hiding this comment.
Well-executed test infrastructure refactoring. The three integration test classes (ProducerIT, MultipleSecretsIT, PropertiesSourceIT) each had their own copy of authenticate() and createSecret() — this PR consolidates them into a shared CyberArkTestSupport base class with improved error handling (requireSuccess). Net result: -7 lines despite adding the new base class.
What's good:
- Each test class now explicitly declares the Conjur policy variables it needs via
loadPolicy()in its@BeforeAll, making prerequisites clear and each class independently runnable requireSuccess()validates HTTP status codes — the oldProducerITandMultipleSecretsITauthenticate()implementations didn't check status codes at all- Documentation updated consistently (
.adocandREADME.md) for file-based API key retrieval instead of log parsing - Account name standardized to
myConjurAccountacross all files - Clean
CyberArkVaultConfigurationlocal variable extraction inPropertiesSourceITreduces chained calls
Minor observations (not blocking):
CyberArkTestSupport.authenticate()usesAccept-Encoding: base64(server-side encoding) while the productionConjurClientImpluses client-sideBase64.getEncoder().encodeToString(). Both are functionally equivalent per the Conjur API, but aligning with the production pattern would make the codebase easier to reason about.URLEncoder.encode("root", ...)inloadPolicy()is a no-op since"root"has no characters needing encoding — harmless but unnecessary.
Checklist:
- Tests — JUnit 5,
*IT.javasuffix, noThread.sleep() - Documentation —
.adocandREADME.mdupdated - Commit convention —
CAMEL-24236:prefix - No production code changes — backward compatible
- Security — no hardcoded secrets, API key is commented out in pom.xml
- CI — pending
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.
6fc53ce to
20c9152
Compare
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
There are uncommitted changes |
20c9152 to
3b7aafe
Compare
3b7aafe to
d4219f8
Compare
gnodet
left a comment
There was a problem hiding this comment.
Clean test refactoring that extracts common CyberArk Conjur test setup into a shared base class and adds proper policy loading. The documentation improvements are nice.
A few minor observations:
Documentation consistency: The .adoc file has a mix of docker compose (modern plugin form, used in the newly added lines) and docker-compose (hyphenated legacy form, on existing unchanged lines) within the same code block. The README.md was correctly updated throughout to use docker compose. Consider updating the .adoc for consistency.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 10 tested, 28 compile-only — current: 9 all testedMaveniverse Scalpel detected 38 affected modules (current approach: 9).
|
Closes apache#24236 Signed-off-by: Thomas Diesler <tdiesler@redhat.com> * Update method on how to setup Conjur and obtain an API key (i.e. file not server logs) * Update tests to install their respective policy
d4219f8 to
4b3dc2d
Compare
Description
Target
mainbranch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.AI-assisted contributions
Co-authored-bytrailers) and the PR description identifies the AI tool used.