Pipeline optimization #819
Conversation
Test overhaul
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR is a substantial refactoring of the CI/CD pipeline, replacing SAP Piper actions with direct tool invocations and shifting from environment-variable-based service credentials to CF-bound service bindings via cds bind. The key actionable issues are: CF inputs being interpolated directly into shell (injection risk), the SonarQube token exposed as a CLI argument instead of an env var, and the sonarqube-scan job binding CF services that are never consumed by the Maven build it runs. The PR description also notes several TODOs still marked incomplete, suggesting this is not yet finalized.
PR Bot Information
Version: 1.20.43
- Correlation ID:
a13c01e4-4d61-49d5-a238-c524bdbb8852 - Event Trigger:
pull_request.opened - LLM:
anthropic--claude-4.6-sonnet - Agent Instructions:
- File Content Strategy: Full file content
There was a problem hiding this comment.
The PR is a solid migration away from Piper-based actions and environment-variable credentials toward native GitHub Actions with cds bind-based service bindings. The most important issues raised are: (1) the SonarQube token being interpolated directly into the shell command instead of via an env variable (security), (2) the CF org/space/api inputs being shell-injectable (security), (3) leftover dead cloud-credential env vars in release.yml that expose secrets unnecessarily, and (4) the combinatorial integration-test matrix generating redundant job permutations (2×2×3 = 12 jobs where many combinations test the same thing).
PR Bot Information
Version: 1.20.43
- Agent Instructions:
- File Content Strategy: Full file content
- LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
issue_comment.created - Correlation ID:
edcee31e-6d1d-4db1-9936-80750b6eeea2
There was a problem hiding this comment.
The PR is a solid pipeline modernization — pinning actions to commit SHAs, removing Piper dependencies, and adopting cds bind for service bindings are all good improvements. The main concerns raised are: the integration-tests and sonarqube-scan jobs running fully in parallel with no dependency on unit tests (wasting expensive CF/cloud resources on broken commits), the blackduck-inc/black-duck-security-scan action being the only one still using a mutable tag (@v2) rather than a pinned SHA, and a retry loop ordering bug in cf-bind/action.yml that causes an unnecessary 30-second sleep on the final failed attempt.
PR Bot Information
Version: 1.20.43
- File Content Strategy: Full file content
- LLM:
anthropic--claude-4.6-sonnet - Correlation ID:
16165f03-ac46-4734-ae41-164c89775417 - Agent Instructions:
- Event Trigger:
pull_request.ready_for_review
| - name: BlackDuck Scan | ||
| uses: SAP/project-piper-action@main | ||
| - name: BlackDuck Security Scan | ||
| uses: blackduck-inc/black-duck-security-scan@v2 |
There was a problem hiding this comment.
Bug: blackduck-inc/black-duck-security-scan@v2 is pinned to a mutable tag, not a commit SHA
All other third-party actions in this PR have been pinned to commit SHAs (e.g. actions/setup-java@be666c2fcd27ec809703dec50e508c2fdc7f6654 # v5), which is the security-hardening approach applied consistently throughout this PR. The BlackDuck action still uses a mutable @v2 tag, meaning the action content can change without warning, introducing supply-chain risk.
Should be pinned to a specific commit SHA with a version comment, matching the pattern used everywhere else.
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
| - name: Install @sap/cds-dk | ||
| run: npm i -g @sap/cds-dk | ||
| shell: bash | ||
|
|
||
| - name: Run Tests | ||
| run: mvn test -ntp -B -P skip-integration-tests -Dcds.install-node.skip |
There was a problem hiding this comment.
Naming: The tests job installs @sap/cds-dk globally but uses -P skip-integration-tests, implying the CDS SDK is needed even for unit tests. However, mvn test with skip-integration-tests profile shouldn't require cds at the shell level. If Dcds.install-node.skip already suppresses the Maven-driven node/cds installation, this global npm install may be redundant — or if it IS required, the reason should be documented.
More importantly, the shell: bash is missing on both the Install @sap/cds-dk step and the Run Tests step (lines 68–72). Composite action steps require shell to be specified; however since this is a workflow (not a composite action), the shell defaults to bash on ubuntu-latest. This is fine but inconsistent with the rest of the file where some run steps lack shell: and others have it — not a blocking issue.
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
1 participant
TODO:
Pipeline is passing on my fork's main