Fixed if condition to run also on skipped approval #714
Conversation
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
I've identified a critical logic error in this pull request. While the PR correctly adds the conditional check to the build job (which directly depends on requires-approval), the same condition was incorrectly applied to all downstream jobs that depend on build, not requires-approval.
The jobs integration-tests, sonarqube-scan, scan, and deploy-snapshot all declare needs: build in their configurations, meaning they should check needs.build.result, not needs.requires-approval.result. With the current implementation, these jobs will execute even if the build fails, as long as the approval step succeeded or was skipped. This defeats the purpose of the dependency chain and could lead to running tests, scans, and deployments against failed builds.
Please update the conditional checks for these four jobs to reference the correct dependency.
PR Bot Information
Version: 1.17.5
- Event Trigger:
pull_request.opened - Correlation ID:
c9e03af0-f079-11f0-90c5-9c913e038575 - LLM:
anthropic--claude-4.5-sonnet
|
|
||
| integration-tests: | ||
| name: Integration Tests (Java ${{ matrix.java-version }}) | ||
| if: always() && (needs.requires-approval.result == 'success' || needs.requires-approval.result == 'skipped') |
There was a problem hiding this comment.
Logic Error: Incorrect dependency check for integration-tests job
The integration-tests job depends on build (line 66), not requires-approval. It should check needs.build.result instead of needs.requires-approval.result. With the current condition, this job will run even if the build job fails, as long as approval succeeded/skipped.
| if: always() && (needs.requires-approval.result == 'success' || needs.requires-approval.result == 'skipped') | |
| if: always() && (needs.build.result == 'success' || needs.build.result == 'skipped') |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
|
|
||
| sonarqube-scan: | ||
| name: SonarQube Scan | ||
| if: always() && (needs.requires-approval.result == 'success' || needs.requires-approval.result == 'skipped') |
There was a problem hiding this comment.
Logic Error: Incorrect dependency check for sonarqube-scan job
The sonarqube-scan job depends on build (line 83), not requires-approval. It should check needs.build.result instead of needs.requires-approval.result. With the current condition, this job will run even if the build job fails, as long as approval succeeded/skipped.
| if: always() && (needs.requires-approval.result == 'success' || needs.requires-approval.result == 'skipped') | |
| if: always() && (needs.build.result == 'success' || needs.build.result == 'skipped') |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
|
|
||
| scan: | ||
| name: Blackduck Scan | ||
| if: always() && (needs.requires-approval.result == 'success' || needs.requires-approval.result == 'skipped') |
There was a problem hiding this comment.
Logic Error: Incorrect dependency check for scan job
The scan job depends on build (line 99), not requires-approval. It should check needs.build.result instead of needs.requires-approval.result. With the current condition, this job will run even if the build job fails, as long as approval succeeded/skipped.
| if: always() && (needs.requires-approval.result == 'success' || needs.requires-approval.result == 'skipped') | |
| if: always() && (needs.build.result == 'success' || needs.build.result == 'skipped') |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
|
|
||
| deploy-snapshot: | ||
| name: Deploy snapshot to Artifactory | ||
| if: always() && (needs.requires-approval.result == 'success' || needs.requires-approval.result == 'skipped') |
There was a problem hiding this comment.
Logic Error: Incorrect dependency check for deploy-snapshot job
The deploy-snapshot job depends on build (line 116), not requires-approval. It should check needs.build.result instead of needs.requires-approval.result. With the current condition, this job will run even if the build job fails, as long as approval succeeded/skipped.
| if: always() && (needs.requires-approval.result == 'success' || needs.requires-approval.result == 'skipped') | |
| if: always() && (needs.build.result == 'success' || needs.build.result == 'skipped') |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
Fixed Workflow Condition to Handle Skipped Approval
Bug Fix
🐛 Bug Fix: Updated GitHub Actions workflow conditions to ensure dependent jobs run correctly when the approval step is skipped for internal pull requests.
Changes
.github/workflows/ci.yml: Added conditional logic to all jobs that depend onrequires-approvalto execute when the approval is either successful or skipped. This ensures that internal pull requests from thecap-javaorganization bypass the approval requirement while external pull requests still require approval before running CI jobs.The following jobs now include the condition
if: always() && (needs.requires-approval.result == 'success' || needs.requires-approval.result == 'skipped'):buildintegration-testssonarqube-scanscandeploy-snapshotThis change ensures the CI pipeline functions correctly for both internal and external contributors without blocking internal PRs on unnecessary approval steps.