utafrali commented on code in PR #26120:
URL: https://github.com/apache/camel/pull/26120#discussion_r3932847665
##########
.github/workflows/sonar-scan.yml:
##########
@@ -143,11 +140,11 @@ jobs:
-Dsonar.pullrequest.branch=${{ env.pr_head_ref }}
-Dsonar.pullrequest.base=${{ env.pr_base_ref }}
-Dsonar.pullrequest.key=${{ env.pr_number }}
- -Dsonar.pullrequest.github.repository=apache/camel
+ -Dsonar.pullrequest.github.repository=gnodet/camel
Review Comment:
This hardcodes `-Dsonar.pullrequest.github.repository=gnodet/camel`,
`-Dsonar.projectKey=gnodet_camel`, and `-Dsonar.organization=gnodet`. If this
lands on `apache/camel`, sonar analysis will publish to your personal fork's
Sonar project instead of `apache_camel`. These look like local testing
overrides that shouldn't be committed. Revert to `apache/camel`,
`apache_camel`, `apache`.
##########
.github/workflows/sonar-scan.yml:
##########
@@ -28,11 +28,8 @@ concurrency:
jobs:
sonar:
- # Temporarily disabled until SonarCloud quality gate is adjusted
(INFRA-27808)
if: >
Review Comment:
Removing `github.repository == 'apache/camel'` from the condition means the
workflow will now attempt to run on every fork that receives a `workflow_run`
event. Combined with the hardcoded `gnodet/camel` project key below, this looks
like fork-local plumbing rather than an intentional main-branch change. Keep
the repository guard.
##########
.github/workflows/sonar-build.yml:
##########
@@ -37,15 +37,15 @@ concurrency:
jobs:
build:
- # Temporarily disabled until SonarCloud quality gate is adjusted
(INFRA-27808)
- if: false && github.repository == 'apache/camel'
+ if: true
Review Comment:
`if: false && ...` was replaced by `if: true`, dropping both the INFRA-27808
gate and the `github.repository == 'apache/camel'` guard in one go. The linked
ticket comment above still says the workflow is disabled pending SonarCloud
quality-gate work — either update/remove the comment and confirm INFRA-27808 is
resolved, or keep the repo guard.
##########
.github/actions/incremental-build/incremental-build.sh:
##########
@@ -480,11 +547,11 @@ main() {
fi
fi
- # Fetch the diff (PR diff via API, or git diff for push builds)
+ # Compute the diff using local git history (merge-base for PRs, HEAD~1 for
push builds)
local diff_body
if [ -n "$prId" ]; then
- echo "Fetching PR #${prId} diff..."
- diff_body=$(fetchDiff "$prId" "$repository")
+ echo "Computing diff against origin/${GITHUB_BASE_REF:-main}..."
Review Comment:
Switching PR diff computation from the GitHub API to a local `git diff
merge-base..HEAD` is a meaningful behavior change (previously the diff matched
exactly what GitHub shows on the PR; now it includes any local merge commits or
force-push artifacts differently). Worth calling out in the PR description and
testing on a PR that has been rebased/force-pushed — the two diffs can diverge
when the base branch has moved forward and the PR hasn't been rebased.
##########
.mvn/extensions.xml:
##########
@@ -23,6 +23,6 @@
<extension>
<groupId>eu.maveniverse.maven.scalpel</groupId>
<artifactId>extension3</artifactId>
- <version>0.3.0</version>
+ <version>0.3.3</version>
Review Comment:
Bumping Scalpel from 0.3.0 to 0.3.3 is unrelated to the flaky-test fix
advertised in the PR title. Please split: (1) the CAMEL-24618 test fix, (2) the
Scalpel bump + shadow-comparison wiring, (3) the sonar/pr-build workflow
changes. Right now this PR mixes three independent changes under a title that
only names one.
##########
.github/workflows/full-test-suite.yml:
##########
@@ -0,0 +1,255 @@
+#
Review Comment:
A new 255-line workflow (`full-test-suite.yml`) is landing under a PR titled
"Fix flaky SpringFileAntPathMatcherRemoteFileFilterTest". Split this out — it
deserves its own PR, its own JIRA (or none if infra-only), and its own review.
Bundling it here hides it from reviewers looking at the ticket scope.
##########
.github/workflows/pr-build-main.yml:
##########
@@ -60,7 +60,7 @@ permissions:
jobs:
build:
- if: github.repository == 'apache/camel'
+ if: github.repository == 'apache/camel' || github.repository ==
'gnodet/camel'
Review Comment:
`github.repository == 'apache/camel' || github.repository == 'gnodet/camel'`
hardcodes a personal fork into an apache/camel workflow. If you need to test on
your fork, use `github.repository_owner != ''` or drop the guard locally, not
in the upstream file.
##########
.github/workflows/sonar-build.yml:
##########
@@ -37,15 +37,15 @@ concurrency:
jobs:
build:
- # Temporarily disabled until SonarCloud quality gate is adjusted
(INFRA-27808)
- if: false && github.repository == 'apache/camel'
+ if: true
name: Build for Sonar Analysis
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd #
v6.0.2
- with:
- persist-credentials: false
-
+ - name: Fetch base branch for Scalpel change detection
+ run: |
+ git fetch --deepen=200 2>/dev/null || true
Review Comment:
The `persist-credentials: false` line was removed from the checkout step.
This is a hardening default (OSSF Scorecard recommends it for
`actions/checkout`) — restore it unless there is a specific reason later steps
need the credential helper.
##########
tests/camel-itest/src/test/resources/org/apache/camel/itest/ftp/SpringFileAntPathMatcherRemoteFileFilterTest-context.xml:
##########
@@ -30,7 +30,7 @@
<template id="camelTemplate"/>
<!-- use myFilter as filter to allow setting ANT paths for which files
to scan for -->
- <endpoint id="myFTPEndpoint"
uri="ftp://admin@localhost:${SpringFileAntPathMatcherRemoteFileFilterTest.ftpPort}/antpath?password=admin&recursive=true&delay=10000&initialDelay=2000&filter=#myAntFilter"/>
+ <endpoint id="myFTPEndpoint"
uri="ftp://admin@localhost:${SpringFileAntPathMatcherRemoteFileFilterTest.ftpPort}/antpath?password=admin&recursive=true&delay=10000&initialDelay=0&delete=true&filter=#myAntFilter"/>
Review Comment:
The URI change itself is the right fix, but consider adding
`readLock=changed` (or `readLock=none` with a comment explaining why it's safe
here) alongside `delete=true`. Without a read lock, on a slow FTP handshake the
consumer can begin reading a file that the producer is still writing —
historically a source of a different flake class in the FTP itests. If you've
already ruled this out for this test, leave it, but a one-line XML comment near
`delete=true` explaining the state-contamination fix would help the next reader.
##########
tests/camel-itest/src/test/java/org/apache/camel/itest/ftp/SpringFileAntPathMatcherRemoteFileFilterTest.java:
##########
@@ -56,6 +56,7 @@ public class SpringFileAntPathMatcherRemoteFileFilterTest {
void testAntPatchMatherFilter() throws Exception {
result.expectedBodiesReceived(expectedBody);
+ result.setResultWaitTime(30000);
Review Comment:
Prefer the timed overload `MockEndpoint.assertIsSatisfied(context, 30,
TimeUnit.SECONDS)` in the assertion below instead of calling
`setResultWaitTime(30000)`. It reads more clearly at the assertion site and
keeps the wait budget next to the assertion it applies to. Per the project
guidelines, mock-based waits should use MockEndpoint's native timed API rather
than setter side-effects.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
