Repository navigation
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
4f11a30 to
4874da0
Compare
gnodet
changed the title
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Clean consolidation. Moving dependency analysis into the main PR build and dropping the dedicated job eliminates a ~20-minute CI rebuild and fixes the version-skew bug that was silently present between the workflow and pom.xml. Two observations worth noting for future maintenance:
Hardcoded version in workflow: pilot-plugin:0.9.0 appears in both .github/workflows/pr-build-main.yml and pom.xml pluginManagement. This reproduces the exact pattern that caused the original 0.4.0 vs 0.5.0 skew — the two will drift again on the next upgrade unless the bump is always done in both places together.
Upload step if condition: The artifact upload at line 221 uses if: matrix.java == '25' without always(). Since GHA's implicit success() applies, any earlier step that fails without continue-on-error will silently skip the upload. The deleted dep-check.yml had if: always() on its upload step — worth considering whether if: always() && matrix.java == '25' is the right guard here.
Neither blocks the merge.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
ℹ️ CI did not run targeted module tests. 🔬 Scalpel shadow comparison — Scalpel: 18 of 700 tested, 4 compile-only — current: 0 all testedMaveniverse Scalpel detected 18 affected modules (current approach: 0). Skip-tests mode would test 18 modules (7 direct + 4 downstream), skip tests for 4 (generated code, meta-modules)
|
| Module | Duration | Status |
|---|---|---|
| Camel :: Launcher | 54.3s | SUCCESS |
| Camel :: JPA | 47.9s | SUCCESS |
| Camel :: JBang :: Plugin :: TUI | 44.2s | SUCCESS |
| Camel :: Salesforce | 43.1s | SUCCESS |
| Camel :: JBang :: MCP | 41.0s | SUCCESS |
| Camel :: Component DSL | 31.5s | SUCCESS |
| Camel :: OpenTelemetry (deprecated) | 27.2s | SUCCESS |
| Camel :: Catalog :: Camel Catalog | 22.1s | SUCCESS |
| Camel :: YAML DSL :: Validator | 18.3s | SUCCESS |
| Camel :: YAML DSL | 16.7s | SUCCESS |
| Camel :: JBang :: Plugin :: Validate | 16.2s | SUCCESS |
| Camel :: Docs | 14.8s | SUCCESS |
| Camel :: JBang :: Plugin :: Testing | 13.9s | SUCCESS |
| Camel :: Wasm | 13.4s | SUCCESS |
| Camel :: JBang :: Plugin :: Kubernetes | 11.4s | SUCCESS |
| Camel :: Kamelet Main | 10.5s | SUCCESS |
| Camel :: Salesforce :: Maven Plugin | 8.7s | SUCCESS |
| Camel :: YAML DSL :: Deserializers | 7.0s | SUCCESS |
| Camel :: Catalog :: Camel Route Parser | 6.6s | SUCCESS |
| Camel :: Catalog :: Camel Report Maven Plugin | 6.5s | SUCCESS |
| Camel :: YAML DSL :: Validator Maven Plugin | 5.3s | SUCCESS |
| Camel :: ServiceNow :: Component | 5.1s | SUCCESS |
| Camel :: All Components Sync point | 4.7s | SUCCESS |
| Camel :: ServiceNow :: Maven Plugin | 3.7s | SUCCESS |
| Camel :: Coverage | 2.9s | SUCCESS |
| Camel :: Catalog :: Maven | 2.8s | SUCCESS |
| Camel :: ServiceNow :: Parent | 2.7s | SUCCESS |
| Camel :: Salesforce :: Parent | 2.1s | SUCCESS |
| Camel :: YAML DSL :: Maven Plugins | 1.8s | SUCCESS |
| Camel :: Catalog :: Suggest (deprecated) | 1.8s | SUCCESS |
| Camel :: JBang :: Plugin :: Generate | 1.6s | SUCCESS |
| Camel :: Assembly | 1.6s | SUCCESS |
| Camel :: JBang :: Plugin :: Edit | 1.6s | SUCCESS |
| Camel :: Salesforce :: CodeGen | 1.3s | SUCCESS |
| Camel :: Catalog :: Console | 1.0s | SUCCESS |
| Camel :: JBang :: Integration tests | 0.9s | SUCCESS |
| Camel :: JBang :: Plugin :: MCP | 0.9s | SUCCESS |
| Camel :: Endpoint DSL :: Support | 0.7s | SUCCESS |
| Camel :: JBang :: Plugin :: Route Parser | 0.7s | SUCCESS |
| Camel :: JBang :: Main | 0.6s | SUCCESS |
| Camel :: Catalog :: Dummy Component | 0.6s | SUCCESS |
| Camel :: Launcher :: Container | 0.5s | SUCCESS |
| Camel :: Etc | 0.1s | SUCCESS |
| Camel :: Endpoint DSL | n/a | |
| Camel :: Integration Tests | n/a | |
| Camel :: JBang :: Core | n/a |
Top 20 slowest modules:
Camel :: Launcher(54.3s)Camel :: JPA(47.9s)Camel :: JBang :: Plugin :: TUI(44.2s)Camel :: Salesforce(43.1s)Camel :: JBang :: MCP(41.0s)Camel :: Component DSL(31.5s)Camel :: OpenTelemetry (deprecated)(27.2s)Camel :: Catalog :: Camel Catalog(22.1s)Camel :: YAML DSL :: Validator(18.3s)Camel :: YAML DSL(16.7s)Camel :: JBang :: Plugin :: Validate(16.2s)Camel :: Docs(14.8s)Camel :: JBang :: Plugin :: Testing(13.9s)Camel :: Wasm(13.4s)Camel :: JBang :: Plugin :: Kubernetes(11.4s)Camel :: Kamelet Main(10.5s)Camel :: Salesforce :: Maven Plugin(8.7s)Camel :: YAML DSL :: Deserializers(7.0s)Camel :: Catalog :: Camel Route Parser(6.6s)Camel :: Catalog :: Camel Report Maven Plugin(6.5s)
4874da0 to
8bdc31e
Compare
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks for folding the dependency analysis into the main PR build, it is nice to drop the separate workflow. Note that my earlier approval was on a different commit ("Temporarily disable dependency analysis CI job"), so this review covers the reworked change as a whole.
Blocking: pilot-plugin 0.9.0 is not on Maven Central. The latest release on Central is 0.5.0 (0.6.0, 0.8.0 and 0.9.0 return 404). The build (25, false) job on this PR actually failed that step:
[ERROR] Plugin eu.maveniverse.maven.plugins:pilot-plugin:0.9.0 or one of its dependencies could not be resolved:
Could not find artifact eu.maveniverse.maven.plugins:pilot-plugin:jar:0.9.0 in central
CI is green only because of continue-on-error: true (and | tee), so the step is currently dead weight. Is 0.9.0 still being staged? If so, it may be best to keep this PR open until the release is on Central.
Smaller points:
| tee dep-check-output.txtwithoutset -o pipefail(the defaultbash -eshell has none) means the step's exit code istee's. Once you switch to-Dpilot.action=check, a failing check would still be reported as success.- Running
./mvnw verify -Pdep-check -DskipTestsfrom the root walks the whole reactor again inside the main build job, which adds to every PR's JDK 25 wall-clock (the old separate job had a 90-minute timeout). Do you have a measurement of the added time once the plugin resolves? - The step comment says it runs after
mvn test"so that target/test-classes exists", but-DskipTestsstill compiles tests, so that ordering isn't needed for the reason given.
This review covers project conventions only and does not replace CodeRabbit, Sourcery or SonarCloud.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| <groupId>eu.maveniverse.maven.plugins</groupId> | ||
| <artifactId>pilot-plugin</artifactId> | ||
| <version>0.5.0</version> | ||
| <version>0.9.0</version> |
There was a problem hiding this comment.
pilot-plugin:0.9.0 does not resolve from Central (latest there is 0.5.0), and the JDK 25 job on this PR failed this step with Could not find artifact eu.maveniverse.maven.plugins:pilot-plugin:jar:0.9.0. Is the release still pending?
| run: | | ||
| ./mvnw verify -Pdep-check -DskipTests -Dlicense.skip \ | ||
| --no-transfer-progress --batch-mode \ | ||
| 2>&1 | tee dep-check-output.txt |
There was a problem hiding this comment.
Without set -o pipefail, this step's exit status is tee's, so a failing -Dpilot.action=check would be reported as success later on.
| 2>&1 | tee dep-check-output.txt | |
| 2>&1 | tee dep-check-output.txt; exit ${PIPESTATUS[0]} |
The job was failing (pilot-plugin 0.4.0 crashing on bom-generator-maven-plugin which has test-scoped deps but no src/test/java) while still showing green due to continue-on-error: true, wasting ~20 min per PR with no actionable output. Switch to workflow_dispatch only until properly fixed.
8bdc31e to
16c6e03
Compare
|
Closing in favor of two separate PRs: one to simply disable the workflow, one to properly fix it. |
gnodet
changed the title
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
LGTM. Clean, minimal disable — replacing pull_request: with workflow_dispatch: is the right approach: it stops the broken workflow from running on PRs while keeping the file for reference and allowing manual runs. Removing the ref:, concurrency:, and permissions: blocks is correct cleanup for the new trigger type (no PR context exists under workflow_dispatch:). The explanatory comment accurately describes the root cause.
Previous findings on pr-build-main.yml (hardcoded version, upload if condition) are no longer applicable here — those will be addressed in the follow-up integration PR.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Following up on my earlier review: the branch was force-pushed (2026-10-07 19:41) and now only contains the "Temporarily disable dependency analysis CI job" change to dep-check.yml. The pom.xml and pr-build-main.yml changes are gone, so my two earlier points (pilot-plugin 0.9.0 not resolving from Central, which still lists 0.5.0 as latest, and the | tee exit status) no longer apply to this PR. Please carry them over to the follow-up PR that moves the analysis into the main workflow.
As it stands this PR is practically a duplicate of #27513: the only difference is that this one also drops the ref: ${{ github.event.pull_request.head.sha }} line. The title and description still describe the move into pr-build-main.yml, and your comment says this PR is being closed in favour of two separate ones, but it is still open. Could you close it (or retitle it as the proper fix once that is ready), so that only #27513 goes in for the disable and the two never get merged on top of each other?
This review covers project conventions only and does not replace CodeRabbit, Sourcery or SonarCloud.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Suggesting this one be closed rather than merged — the substance already landed elsewhere, and it's currently sitting on a stale block.
The prior CHANGES_REQUESTED no longer applies — it should be dismissed, not satisfied
@davsclaus' block (2026-10-07T18:17) was against commit 8bdc31e3eaf6, which touched pom.xml and .github/workflows/pr-build-main.yml, and raised: (1) pilot-plugin:0.9.0 not resolving from Central, masked by continue-on-error: true; (2) | tee without set -o pipefail; (3) re-walking the whole reactor inside the main build job; (4) the step comment's target/test-classes justification being wrong under -DskipTests.
The branch was then force-pushed (2026-10-07 19:41). The current head touches only .github/workflows/dep-check.yml — pom.xml and pr-build-main.yml are no longer in the diff, so points 1-4 have nothing to attach to. @davsclaus confirmed this himself in a follow-up COMMENTED review (2026-10-08T05:42).
The change is now a 1-line residue
#27513 "Temporarily disable dependency analysis CI job" merged 2026-10-08T08:46 (ba221da9a8dc), and origin/main:.github/workflows/dep-check.yml already carries the identical disable comment block and on: workflow_dispatch:.
Verified net remaining delta (git diff origin/main refs/remotes/pr/27506 -- .github/workflows/dep-check.yml) — exactly one line, at dep-check.yml:40:
- name: Checkout
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
- ref: \${{ github.event.pull_request.head.sha }}
fetch-depth: 1A legitimate (tiny) cleanup — under workflow_dispatch there's no github.event.pull_request, so the expression evaluates to empty and actions/checkout silently falls back to the default ref. Dead and misleading, but harmless. (gh pr diff still shows +7/-22 because it diffs against the merge base, which predates #27513 — that's why the PR looks bigger than it is.)
CI is green but hollow: "Build and test" never ran, because the only changed file is under .github/**, which is in pr-build-main.yml:28 paths-ignore. mergeStateStatus: BLOCKED is the stale review, not git.
If you keep it open instead
[Important] Title and description no longer describe the change. The title says "Move dependency analysis into main PR workflow" and the body describes adding a pilot step to pr-build-main.yml and binding the goal to verify in the dep-check profile — none of which is in the diff. Per the PR-description-maintenance rule it'd need rewriting to something like "remove stale ref: from the disabled dep-check workflow".
[Suggestion] No JIRA reference. The file's own history is predominantly JIRA-tracked — CAMEL-24985: ci - simplify Dependency Analysis workflow (88d59f944fd9), ci: temporarily disable Dependency Analysis workflow (CAMEL-24985) (48e5e75bd9b1), CAMEL-22967: Add dep-check profile using pilot:dependencies (abff56356bb8). CAMEL-24985 is the precedent for this exact workflow. That said, #27513 went in with no prefix either, so practice for throwaway CI toggles is mixed — raising it as a suggestion. The real follow-up (moving pilot into pr-build-main.yml) should definitely carry a ticket, since it changes every PR's build.
Carry these two forward to the real follow-up PR
Both are correct and will recur:
pilot-pluginversion must exist on Central before the step is added, and must not be hardcoded in bothpr-build-main.ymlandpom.xmlpluginManagement— that duplication is what produced the original 0.4.0/0.5.0 skew.… 2>&1 | tee dep-check-output.txtneedsexit \${PIPESTATUS[0]}(orset -o pipefail), otherwise the step's exit status istee's and a real-Dpilot.action=checkfailure passes silently. GitHub's default shell isbash -e, which does not implypipefail.
One thing to confirm before merging anything that drops a permissions: block: the current head removes permissions: contents: read from the workflow. That's acceptable only because the block came with the deleted pull_request trigger and the repo default then applies — but I could not verify apache/camel's org/repo default GITHUB_TOKEN permission setting (needs admin API access). Worth a check.
Reviewed with Claude Code (Claude Opus 5) on behalf of @oscerd. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying. It is a rules-and-conventions review and does not replace CodeRabbit, Sourcery, or SonarCloud.
4 participants
The standalone
dep-check.ymlworkflow had two problems:continue-on-error: true: pilot-plugin 0.4.0 crashes on modules that declare test-scoped deps but have nosrc/test/java(e.g.bom-generator-maven-plugin)Changes
dep-check.yml— no more redundant standalone rebuild (the disable is tracked in Temporarily disable dependency analysis CI job #27513)pr-build-main.yml— runs only on the JDK 25 matrix entry, aftermvn test(sotarget/test-classesexists for correct test-scope analysis). No extra build cost.verifyphase in thedep-checkprofile so the workflow just calls./mvnw verify -Pdep-check -DskipTests: version lives only inpom.xmlpluginManagement, Dependabot can track it there.continue-on-error: true/ report mode — flip to-Dpilot.action=checkonce the report is clean.