Copilot commented on code in PR #17316:
URL: https://github.com/apache/pinot/pull/17316#discussion_r2586736526
##########
.github/workflows/pinot_tests.yml:
##########
@@ -235,7 +265,12 @@ jobs:
verbose: true
- name: Generate Surefire Report
if: ${{ !cancelled() && github.ref == 'refs/heads/master' }}
- run: mvn surefire-report:report-only -P
github-actions,codecoverage,no-integration-tests || echo "Surefire report
generation failed, but continuing..."
+ uses: nick-fields/retry@v3
+ with:
+ max_attempts: 3
+ retry_wait_seconds: 20
+ timeout_minutes: 120
+ command: mvn surefire-report:report-only -P
github-actions,codecoverage,no-integration-tests || echo "Surefire report
generation failed, but continuing..."
Review Comment:
The command includes `|| echo \"Surefire report generation failed, but
continuing...\"` which will cause the retry action to consider any Maven
failure as a success. This defeats the purpose of adding retry logic. Either
remove the `|| echo` fallback to allow retries on failure, or add
`continue-on-error: true` to the step configuration if failure should be
tolerated without retries.
```suggestion
command: mvn surefire-report:report-only -P
github-actions,codecoverage,no-integration-tests
```
##########
.github/workflows/pinot_tests.yml:
##########
@@ -100,22 +110,32 @@ jobs:
${{ runner.os }}-maven-
# Build the PR branch's module to get a new JAR to compare
- name: Build SPI on PR branch
- run: |
- for mod in pinot-spi pinot-segment-spi; do
- mvn clean package -T1C -pl $mod -am -DskipTests
-Dproject.build.finalName="$mod"
- done
+ uses: nick-fields/retry@v3
+ with:
+ max_attempts: 3
+ retry_wait_seconds: 20
+ timeout_minutes: 120
+ command: |
+ for mod in pinot-spi pinot-segment-spi; do
+ mvn clean package -T1C -pl $mod -am -DskipTests
-Dproject.build.finalName="$mod"
+ done
- name: Checkout master into a subfolder
uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.base.sha }}
path: master-head
# Build the same module on master to produce the baseline JAR
- name: Build SPI on master
- working-directory: master-head
- run: |
- for mod in pinot-spi pinot-segment-spi; do
- mvn clean package -T1C -pl $mod -am -DskipTests
-Dproject.build.finalName="$mod"
- done
+ uses: nick-fields/retry@v3
+ with:
+ max_attempts: 3
+ retry_wait_seconds: 20
+ timeout_minutes: 120
+ command: |
+ cd master-head
Review Comment:
The `working-directory` context has been removed but the command now
includes `cd master-head`. This change is correct, but the command will fail if
subsequent Maven commands in lines 136-138 don't execute in that directory.
Since the `command` field executes as a single shell script, this should work,
but verify that the `for` loop and Maven commands execute within the
`master-head` directory context.
##########
.github/workflows/pinot_tests.yml:
##########
@@ -366,7 +416,12 @@ jobs:
verbose: true
- name: Generate Surefire Report
if: ${{ !cancelled() && github.ref == 'refs/heads/master' }}
- run: mvn surefire-report:report-only -P github-actions,codecoverage ||
echo "Surefire report generation failed, but continuing..."
+ uses: nick-fields/retry@v3
+ with:
+ max_attempts: 3
+ retry_wait_seconds: 20
+ timeout_minutes: 120
+ command: mvn surefire-report:report-only -P
github-actions,codecoverage || echo "Surefire report generation failed, but
continuing..."
Review Comment:
Same issue as Comment 2: the `|| echo` fallback prevents the retry mechanism
from working. The command will always succeed even if Maven fails, so retries
will never trigger. Consider removing the fallback or using `continue-on-error:
true` instead.
```suggestion
command: mvn surefire-report:report-only -P
github-actions,codecoverage
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
