AAP-81136 - Add workflow for SonarCloud reporting - #45
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds SonarCloud project configuration and a post-CI GitHub Actions workflow for pull request and branch analysis, including coverage discovery, repository metadata, changed-file inclusions, and commit-specific scanning. ChangesSonarCloud analysis
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant SonarWorkflow
participant GitHubAPI
participant SonarCloud
CI->>SonarWorkflow: Complete successfully
SonarWorkflow->>GitHubAPI: Fetch PR refs and changed files
GitHubAPI-->>SonarWorkflow: Return PR metadata
SonarWorkflow->>SonarCloud: Run PR scan with revision and inclusions
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/sonarcloud_pr.yml:
- Around line 47-49: Add persist-credentials: false to both actions/checkout@v4
steps in the workflow, including the checkout near the coverage artifact
download and the later checkout around line 210. Keep the existing checkout
behavior unchanged otherwise.
- Around line 84-137: The PR-number handling is contradictory: the fatal exit
before the changed-files logic makes the “PR Number not available” fallback
unreachable. Update the PR metadata flow around the PR_NUMBER validation and
changed-files conditional so missing PR_NUMBER follows the existing full-scan
fallback, or remove that fallback if missing PR numbers must remain fatal;
ensure only the intended behavior remains and avoid exporting or using an
unavailable PR number.
- Line 37: Replace the workflow-wide permissions: read-all declaration with an
explicit least-privilege permissions block. Grant only the scopes required by
the workflow, such as contents: read, actions: read for artifact downloads, and
pull-requests: read, while preserving any permissions needed by the jobs.
- Around line 193-199: Update the SonarCloud pull-request workflow args to
conditionally add -Dsonar.python.coverage.reportPaths only when
env.COVERAGE_PATHS is non-empty, matching the existing branch-job behavior;
preserve the other scanner arguments unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 5b1c0c76-5a84-4e00-b8dd-256ad695d4a7
📒 Files selected for processing (1)
.github/workflows/sonarcloud_pr.yml
c5d8985 to
9522b88
Compare
9522b88 to
c2dc2c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/sonarcloud_pr.yml:
- Around line 37-40: Update the top-level permissions block to replace the
invalid pull_requests scope with the valid pull-requests scope, preserving the
existing contents and actions permissions.
- Around line 55-61: Update the “Download coverage artifacts” step using
dawidd6/action-download-artifact so missing api-test-artifacts do not fail the
workflow; enable continue-on-error or configure if_no_artifact_found to a
non-failing mode, while preserving the existing artifact selection and later
fallback behavior.
- Around line 175-181: Update the “Add base branch” workflow step to validate
PR_NUMBER as numeric before using it and pass the validated value quoted to
GitHub CLI/API commands. Pin checkout to github.event.workflow_run.head_sha
instead of the PR’s current head, ensuring SonarCloud analyzes the exact
CI-tested commit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 3361eed7-30eb-4eb3-a28f-8269591a2ce4
📒 Files selected for processing (1)
.github/workflows/sonarcloud_pr.yml
SummaryThis PR adds a single GitHub Actions workflow file (
Overall AssessmentThe There are several issues to address -- some already flagged by CodeRabbit (see cross-references below), and some additional ones. SonarCloud Onboarding -- only partially addressedThe ticket describes 3 steps:
Gap: No
|
| # | Source | Severity | Status | Description |
|---|---|---|---|---|
| CR-1 | CodeRabbit | Critical | Open | pull_requests -> pull-requests (invalid scope) |
| CR-2 | CodeRabbit | Major | Open | Artifact download should tolerate missing artifacts |
| CR-3 | CodeRabbit | Critical | Open | Validate PR_NUMBER + pin checkout to tested SHA |
| A-1 | This review | Low | New | actions/checkout@v4 not SHA-pinned |
| A-2 | This review | Major | New | PR number extraction from coverage.xml is fragile |
| A-3 | This review | Low | New | Duplicate summary block |
| A-4 | This review | Medium | New | Fork code checked out with access to secrets |
| A-5 | This review | Medium | New | Missing sonar-project.properties file |
| A-6 | This review | Low | New | Hardcoded .coveragerc patterns will drift |
| A-7 | This review | Low | New | Redundant COMMIT_SHA env export |
Recommendation: Address CR-1 (blocker -- invalid permission scope), CR-2, A-2, and A-5 before merge. The rest are improvements that can be addressed in a follow-up.
There was a problem hiding this comment.
Code Review - made possible by Claude Qodo PR Review Skill
The workflow structure is solid — using workflow_run for fork-safe SonarCloud analysis with pinned action SHAs is the right approach. Found a few items worth addressing, mostly around API pagination, checkout configuration, and log accuracy.
thedoubl3j
left a comment
thedoubl3j
left a comment
There was a problem hiding this comment.
Similar to Dirk's review, a project properties file is needed as part of this. That can also be lifted from AWX and changed appropriately for this repo.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
sonar-project.properties (2)
106-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment doesn't match the rule it describes.
python:S1192flags duplicated string literals, not "should be a variable in migrations." The stale/incorrect comment could confuse future maintainers about why this rule is suppressed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sonar-project.properties` around lines 106 - 108, Update the comment above sonar.issue.ignore.multicriteria.e1.ruleKey to accurately describe that python:S1192 suppresses duplicated string literal findings, removing the incorrect “should be a variable in migrations” description while leaving the rule configuration unchanged.
70-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffExclusion patterns duplicated across config and workflow.
sonar.exclusions/sonar.coverage.exclusionshere overlap with the coverage-omit filtering logic hardcoded in the workflow (.github/workflows/sonarcloud_pr.yml), which independently greps forsettings/.*_defaults\.py,main/migrations/, etc. Keeping both in sync manually risks drift; consider deriving the workflow's exclusion list from this file (or.coveragerc) instead of hardcoding it separately.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sonar-project.properties` around lines 70 - 99, Centralize SonarCloud exclusion handling by updating the workflow’s coverage-omit filtering to derive patterns from the existing sonar-project.properties or .coveragerc configuration instead of maintaining a separate hardcoded list. Preserve the current exclusions, including settings defaults and migrations, while removing the duplicated workflow-specific pattern definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sonar-project.properties`:
- Around line 55-56: Update the sonar.python.xunit.reportPath setting to use the
same project-relative path format as sonar.python.coverage.reportPaths by
removing the leading slash from the JUnit report path.
---
Nitpick comments:
In `@sonar-project.properties`:
- Around line 106-108: Update the comment above
sonar.issue.ignore.multicriteria.e1.ruleKey to accurately describe that
python:S1192 suppresses duplicated string literal findings, removing the
incorrect “should be a variable in migrations” description while leaving the
rule configuration unchanged.
- Around line 70-99: Centralize SonarCloud exclusion handling by updating the
workflow’s coverage-omit filtering to derive patterns from the existing
sonar-project.properties or .coveragerc configuration instead of maintaining a
separate hardcoded list. Preserve the current exclusions, including settings
defaults and migrations, while removing the duplicated workflow-specific pattern
definitions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 46faf938-26fa-45f1-b8fb-1c7b8e8029d6
📒 Files selected for processing (2)
.github/workflows/sonarcloud_pr.ymlsonar-project.properties
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/sonarcloud_pr.yml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/sonarcloud_pr.yml (1)
51-54: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winPR job checkout never actually fetches the tested commit —
git checkout "$COMMIT_SHA"will fail.The initial checkout (Lines 51-54) has no
ref:, so it checks out the default branch (theworkflow_runcontext always resolves to the default branch unlessref:is set — exactly why the branch job at Line 198 was fixed to useref: ${{ github.event.workflow_run.head_sha }}). Later, Lines 163-168 try to compensate by runninggit fetch origin "$PR_BASE"thengit checkout "$COMMIT_SHA". Fetching the base branch by name does not bring the PR head commit into the local repo — that commit lives on a different branch (and for fork PRs, isn't even a branch oforiginat all). Unless$COMMIT_SHAhappens to already be an ancestor of the default branch (e.g., already merged), this checkout will fail withfatal: reference is not a tree, breaking the PR analysis job for essentially every open PR.Mirror the branch job's fix: pin
ref:on the initial checkout to the tested commit. GitHub allows fetching arbitrary reachable SHAs (this is exactly how the branch job'sref: head_shapattern already works for fork-triggeredworkflow_run).🐛 Proposed fix
- uses: actions/checkout@v4 with: + ref: ${{ github.event.workflow_run.head_sha }} fetch-depth: 0 persist-credentials: false# Fetch the PR branch for SonarCloud PR analysis comparison git fetch origin "$PR_BASE" - - # Checkout the exact commit that CI tested, not the current PR head - # This ensures SonarCloud analyzes the same code that CI validated - git checkout "$COMMIT_SHA" + # Already checked out at $COMMIT_SHA via the initial checkout step's `ref:`.Also applies to: 163-168
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/sonarcloud_pr.yml around lines 51 - 54, Pin the initial PR checkout in the workflow’s actions/checkout@v4 step to the tested workflow_run head commit by adding the appropriate ref value, matching the existing branch-job pattern. Ensure the later git checkout of COMMIT_SHA can resolve the PR head, including commits from forked repositories, without relying on fetching PR_BASE.
🧹 Nitpick comments (2)
.github/workflows/sonarcloud_pr.yml (2)
51-54: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin
actions/checkoutto a commit SHA for consistency with the other pinned actions in this workflow.
dawidd6/action-download-artifactandSonarSource/sonarqube-scan-actionare already pinned to commit SHAs, but bothactions/checkout@v4uses (Line 51, Line 196) still use a mutable tag. For supply-chain consistency and to avoid a compromised tag silently swapping in different behavior.uses: actions/checkout@<pinned-sha> # v4Also applies to: 196-200
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/sonarcloud_pr.yml around lines 51 - 54, Pin both actions/checkout usages in the workflow, including the steps around the existing fetch-depth settings, to an immutable commit SHA while retaining the v4 version comment. Do not change the checkout configuration or the other already-pinned actions.
172-184: 🔒 Security & Privacy | 🔵 TrivialScanning PR/fork code with access to org secrets deserves an explicit mitigation note.
This workflow runs via
workflow_run, so it executes with base-repo permissions and has access toSONAR_TOKEN/GITHUB_TOKENwhile operating on code checked out from the PR (potentially a fork, once the checkout fix above is applied to actually pull the PR's commit). This is the classic "pwn request" pattern: if anything in the scan pipeline (custom Sonar plugins, build hooks invoked by the scanner, etc.) executes attacker-controlled repo content, secrets could be exfiltrated. Worth documenting the trust boundary explicitly (e.g., confirm the SonarCloud scanner action doesn't execute repo-provided build/config scripts, and thatSONAR_TOKEN/GITHUB_TOKENscopes are minimal) since this is called out as an open concern in the PR objectives.Also applies to: 229-238
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/sonarcloud_pr.yml around lines 172 - 184, Document the workflow_run trust boundary near the SonarCloud Scan step, explicitly noting that PR or fork code is scanned with base-repository GITHUB_TOKEN and SONAR_TOKEN access. Record the required mitigations: verify the scanner does not execute repository-controlled build/config scripts, and ensure both token permissions and SonarCloud token scope are minimal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/sonarcloud_pr.yml:
- Around line 51-54: Pin the initial PR checkout in the workflow’s
actions/checkout@v4 step to the tested workflow_run head commit by adding the
appropriate ref value, matching the existing branch-job pattern. Ensure the
later git checkout of COMMIT_SHA can resolve the PR head, including commits from
forked repositories, without relying on fetching PR_BASE.
---
Nitpick comments:
In @.github/workflows/sonarcloud_pr.yml:
- Around line 51-54: Pin both actions/checkout usages in the workflow, including
the steps around the existing fetch-depth settings, to an immutable commit SHA
while retaining the v4 version comment. Do not change the checkout configuration
or the other already-pinned actions.
- Around line 172-184: Document the workflow_run trust boundary near the
SonarCloud Scan step, explicitly noting that PR or fork code is scanned with
base-repository GITHUB_TOKEN and SONAR_TOKEN access. Record the required
mitigations: verify the scanner does not execute repository-controlled
build/config scripts, and ensure both token permissions and SonarCloud token
scope are minimal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2eb299ae-6171-4701-beff-c496fa8bfda8
📒 Files selected for processing (2)
.github/workflows/sonarcloud_pr.ymlsonar-project.properties
🚧 Files skipped from review as they are similar to previous changes (1)
- sonar-project.properties
There was a problem hiding this comment.
(personal request, preexisting convention)
Could you use kebab-case style name for this file?
There was a problem hiding this comment.
This piece of automation doesn't seem unique enough to be attached to the project — I'd maybe look into having a composite action or a reusable workflow in a shared repository rather than here.
There was a problem hiding this comment.
While I agree that it should be, the ask to implement sonar has been quick and delivery wants to be quick. I do think its something that we could look into but I think that would need to be at an org level.
| # 7363/32 | ||
| name: SonarCloud | ||
| on: | ||
| workflow_run: # This is triggered by CI being completed. |
There was a problem hiding this comment.
Another approach could be having "modules" of form reusable-*.yml plugged into ci-cd.yml.
There was a problem hiding this comment.
Do you have an example of what this would look like? Would they be a job that uses the reusable-*yml like https://github.com/ansible/awx_plugins.interfaces/blob/devel/.github/workflows/ci-cd.yml#L336? Do we want them stored under /reusable or in the potential new shared repo for these workflows?
5 participants
JIRA: https://redhat.atlassian.net/browse/AAP-81136
Add workflow for automatic SonarCloud code analysis.
Summary by CodeRabbit