Skip to content

5348 sonar qube pr problem#780

Merged
ToreMerkely merged 10 commits intomainfrom
5348-sonar-qube-pr-problem
Apr 15, 2026
Merged

5348 sonar qube pr problem#780
ToreMerkely merged 10 commits intomainfrom
5348-sonar-qube-pr-problem

Conversation

@ToreMerkely
Copy link
Copy Markdown
Contributor

  • Added hard coded test for sonarqube PR scan to reproduce error server/#5348
  • Fixed problem and change to use kosli attest sonar for test

@ToreMerkely ToreMerkely enabled auto-merge (squash) April 15, 2026 10:57
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 15, 2026

Claude finished @ToreMerkely's task in 2m 38s —— View job


PR Review: #780 — SonarQube PR Scan Fix

  • Run static analysis (build, vet, mod tidy)
  • Read and review changed files
  • Post review findings

Static Analysis

All checks pass: go build ./..., go vet ./..., go mod tidy — no issues.


Summary

This PR fixes SonarQube PR scan support (server/#5348) by:

  1. Detecting pull request scans from CE task data and routing to a new GetPRAnalysisData function that uses the project_pull_requests/list API
  2. Adding a --sonar-ce-task-url flag for CI environments without filesystem access to report-task.txt
  3. Fixing use-before-error-check on http.NewRequest results across all sonar functions
  4. Adding missing defer Body.Close() in GetProjectAnalysisFromAnalysisID
  5. Fixing comment accuracy and updating test fixture documentation

The core fix is well-structured — the PR/branch detection in GetCETaskData (lines 354-363) cleanly routes to the appropriate downstream function.


Issues

Pre-existing: Response body leaks (not introduced by this PR)

Three functions still have response body close issues. These are pre-existing but worth cleaning up since the file is already being improved:

  1. GetCETaskData retry loop (lines 289-327): When task status is PENDING/IN_PROGRESS, taskResponse.Body is never closed before the next iteration. Each retry leaks an open body. The break path closes it, but the retry path doesn't. Add taskResponse.Body.Close() after decoding in each iteration.

  2. GetQualityGate (line 509): qualityGateResponse.Body is never closed. Add defer func() { _ = qualityGateResponse.Body.Close() }() after the httpClient.Do call, matching the pattern in GetPRAnalysisData.

  3. GetTaskID (lines 552-581): Body is closed manually at the bottom (line 578) but not via defer, so the decode error path on line 560 leaks. Switch to defer and remove the manual close.

Fix all three body close issues →

Nit: GetProjectAnalysisFromRevision also missing body close on error paths

GetProjectAnalysisFromRevision (line 407) closes the body manually at the end, but early returns from the decode error (line 389) and error check (line 401) skip the close. The defer pattern would be safer here too.


What's Good

  • Clean PR/branch detection: The PullRequest field handling in GetCETaskData (lines 354-363) correctly prioritizes PR over branch, with explicit nil-ing of the alternative field.
  • New GetPRAnalysisData function (lines 460-496): Well-structured, uses defer for body close, checks for API errors before iterating, and has a clear "not found" error message. This is the best body-close pattern in this file.
  • Thorough documentation: The long description clearly explains all three methods, with the method-2 limitation called out. The updated update-sonarqube-test-data.txt with PR fixture instructions is a nice touch.
  • --sonar-ce-task-url flag: Good addition for container-isolated CI environments. The server URL extraction from the CE task URL (lines 183-187) is clean.
  • Consistent error-check fixes: All five http.NewRequest use-before-error-check bugs are fixed consistently.
  • Test coverage: Two new test cases exercise both the PR scan path and the new --sonar-ce-task-url flag.

Comment thread cmd/kosli/attestSonar_test.go
Comment thread internal/sonar/sonar.go
ToreMerkely and others added 2 commits April 15, 2026 13:10
…t-task.txt access

Allows users to pass the SonarQube CE task URL directly, bypassing the
need for the .scannerwork/report-task.txt file. This solves the case
where the scanner and the Kosli CLI run in different containers (e.g.
Jenkins with Kubernetes pod agents) and don't share a filesystem.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment thread internal/sonar/sonar.go
Comment thread internal/sonar/sonar.go
ToreMerkely and others added 2 commits April 15, 2026 13:31
Reorder the error check to come before taskRequest.Header.Add() to
prevent a nil pointer dereference if NewRequest fails. Pre-existing
bug, but now more reachable via the new --sonar-ce-task-url flag.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment thread internal/sonar/sonar.go
…ctions

Same nil pointer dereference pattern as fixed in GetCETaskData, now
fixed in GetProjectAnalysisFromRevision, GetProjectAnalysisFromAnalysisID,
GetQualityGate, and GetTaskID.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment thread internal/sonar/sonar.go
Comment thread cmd/kosli/attestSonar_test.go
Comment thread internal/sonar/sonar.go Outdated
ToreMerkely and others added 2 commits April 15, 2026 13:47
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add instructions for updating the PR scan test fixture to
update-sonarqube-test-data.txt. Fix comment on readFile that
incorrectly claimed it extracts the project key.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment thread internal/sonar/sonar.go
Comment thread internal/sonar/sonar.go
Comment thread cmd/kosli/attestSonar.go
Comment thread internal/sonar/sonar.go
Response body was never closed, leaking connections. Also moved the
error check before the loop for consistency with GetPRAnalysisData.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ToreMerkely ToreMerkely merged commit 87877c1 into main Apr 15, 2026
20 checks passed
@ToreMerkely ToreMerkely deleted the 5348-sonar-qube-pr-problem branch April 15, 2026 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants