docs(skill): rewrite sonarqube cleanup section around tiered bulk fixes
SonarQube Analysis / sonarqube (pull_request) Successful in 2m51s
SonarQube Analysis / sonarqube (pull_request) Successful in 2m51s
Section A (server inspection / API access) is unchanged. Section B is rewritten around the tiered model: - Tier 1 — mechanical: bulk fix multiple rule IDs in one dotnet format invocation (IDE0028, CA1825, CA1834, etc). - Tier 2 — judgment: one commit per rule, audit diff before staging. Calls out CA1822 public-static = binary break, CA1861 cryptic generated field names + duplicate-field collisions, CA1859 has no batch fixer. - Tier 3 — manual / no fixer: csharpsquid:Sxxxx, public-API design calls. Reference [[sonarqube-wontfix-rules]] memory for triage. Adds the local-Java-version gotcha: `./scan-sonar.ps1` is blocked here (system JRE is 8, scanner needs 17); CI handles the upload via actions/setup-java@v4 with temurin 17. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Thom Lamb
co-authored by
Claude Opus 4.7
Claude Opus 4.7
parent
6af239035b
commit
c38d122d76
@@ -38,29 +38,74 @@ Invoke-RestMethod -Uri "$url/api/issues/search?componentKeys=sql-utilities&resol
|
||||
|
||||
`SONARQUBE_URL` / `SONARQUBE_TOKEN` are read here; `scan-sonar.ps1` reads the separate `SONAR_TOKEN` / `SONAR_HOST_URL` for the *upload* path — do not conflate them.
|
||||
|
||||
## B — Cleanup loop
|
||||
## B — Cleanup loop (tiered)
|
||||
|
||||
```dot
|
||||
digraph cleanup {
|
||||
query [label="Query open issues + facets"];
|
||||
pick [label="Pick one rule group"];
|
||||
edit [label="Edit code"];
|
||||
build [label="dotnet build -c Release"];
|
||||
test [label="dotnet test -c Release --no-build"];
|
||||
commit [label="git commit\nchore(sonar): … (Sxxxx)"];
|
||||
more [label="More groups?" shape=diamond];
|
||||
scan [label="scan-sonar.ps1"];
|
||||
verify [label="Query API: confirm net-down"];
|
||||
query [label="Query open issues + facets=rules"];
|
||||
triage [label="Tier each rule:\nmechanical / judgment / API-impact"];
|
||||
t1 [label="Tier 1 — mechanical:\nbulk-fix in one commit"];
|
||||
t2 [label="Tier 2 — judgment:\none commit per rule, audit diff"];
|
||||
t3 [label="Tier 3 — API-impact:\nmanual, accept !breaking or skip"];
|
||||
build [label="dotnet build + dotnet test\nafter EACH commit"];
|
||||
push [label="git push + open PR"];
|
||||
ci [label="CI runs SonarScanner\non Java 17"];
|
||||
verify [label="Re-query API:\nconfirm rule counts dropped"];
|
||||
|
||||
query -> pick -> edit -> build -> test -> commit -> more;
|
||||
more -> pick [label="yes"];
|
||||
more -> scan [label="no"];
|
||||
scan -> verify;
|
||||
query -> triage;
|
||||
triage -> t1 -> build;
|
||||
triage -> t2 -> build;
|
||||
triage -> t3 -> build;
|
||||
build -> push -> ci -> verify;
|
||||
}
|
||||
```
|
||||
|
||||
1. **Query.** Start with `severities=MAJOR,MINOR` + `facets=rules` to see what's worth fixing.
|
||||
2. **Pick a rule group.** One Sxxxx (or one tightly-related cluster) per commit, easiest first so a late blocker doesn't strand the others.
|
||||
3. **Edit → build → test.** Build must succeed; the warning count for the touched rule must drop. Tests must stay green. If a fix would degrade clarity or break a tested contract, prefer **Won't Fix on the server with a justification** over a forced code change — see [[sonarqube-wontfix-rules]] for the catalog of rules already triaged that way (S1168, S3925, CS8601, S107).
|
||||
4. **Commit.** `chore(sonar): <one-line> (Sxxxx)` — bang (`!`) if it's a breaking rename.
|
||||
5. **After all groups:** `./scan-sonar.ps1` (needs `$env:SONAR_TOKEN`). Wait ~30-60s for the CE task to finish, then re-query the issues endpoint and confirm the open MAJOR/MINOR count fell by the expected number.
|
||||
**The loop has shifted from per-rule commits to *tiered batches* once a project has more than a handful of issues left.** Use `dotnet format analyzers --diagnostics <ids> --severity info` — it runs all the Roslyn-shipped code fixers for the listed diagnostics, including `CA*`, `IDE*`, and `NUnit*`. (The `csharpsquid:Sxxxx` family from SonarAnalyzer.CSharp usually has no `dotnet format` fixer; those still need manual edits.)
|
||||
|
||||
### Tier 1 — mechanical (one bulk commit)
|
||||
|
||||
Rules where the fixer's rewrite is purely syntactic and can't change behavior — pile them into one `dotnet format` invocation, audit the diff for sanity, build, test, single commit:
|
||||
|
||||
```powershell
|
||||
dotnet format analyzers Strata.SqlTools.QueryBreakdown.sln `
|
||||
--diagnostics IDE0028 CA1825 CA1834 CA1845 CA1847 CA1860 CA1866 CA1853 CA1830 CA2249 `
|
||||
--severity info --verbosity normal
|
||||
```
|
||||
|
||||
Typical safe rules: `IDE0028`, `CA1825`, `CA1834`, `CA1845`, `CA1847`, `CA1853`, `CA1860`, `CA1866`, `CA2249`, `CA1830`. Some rules in this family report "no associated code fix" — they'll need manual handling separately.
|
||||
|
||||
### Tier 2 — judgment (one commit per rule)
|
||||
|
||||
Rules whose fixer can produce mediocre output or surprise the reader — bulk-fix but **audit before staging**:
|
||||
|
||||
- `CA1510` — `ArgumentNullException.ThrowIfNull` rollup; always safe but voluminous, deserves its own commit.
|
||||
- `CA1822` — make-method-static; the fixer also rewrites internal callers to use type-name form. **Public methods becoming static are a binary-break for external NuGet consumers** — commit with `chore(sonar)!:` and a `BREAKING CHANGE:` footer naming each affected member, or revert those file diffs and apply only the private-helper changes.
|
||||
- `CA1861` — hoists constant array args to `static readonly` fields. The fixer's field names are sometimes cryptic (`stringArray`, `arg2`) or it emits duplicate `separator` fields in the same class. After running, rename ugly fields and dedupe collisions before committing.
|
||||
- `CA1854` — `TryGetValue`-style double-lookup elimination; trivial.
|
||||
- `CA1859` — concrete return type for perf. **No batch fixer**: edit each site manually after checking visibility. Apply only to private/internal/test; on public/protected, either revert or treat as a `!breaking` change (most cases are private helpers, so this is usually fine).
|
||||
|
||||
### Tier 3 — manual (no fixer)
|
||||
|
||||
`CA1806`, `CA1846`, `CA1869`, the `csharpsquid:Sxxxx` family that has no `dotnet format` fixer, and individual public-API rules that need design judgment. Edit by hand, one rule at a time. If a fix would degrade clarity or break a tested contract, prefer **server-side Won't Fix with justification** — see [[sonarqube-wontfix-rules]] for the catalog already triaged that way (S1168, S3925, CS8601, S107).
|
||||
|
||||
### Guardrail
|
||||
|
||||
After every commit (Tier 1, 2, or 3):
|
||||
|
||||
```powershell
|
||||
dotnet build Strata.SqlTools.QueryBreakdown.sln -c Release --nologo
|
||||
dotnet test Strata.SqlTools.QueryBreakdown.sln -c Release --no-build --nologo
|
||||
```
|
||||
|
||||
Build succeeds with no new warnings beyond the baseline. Tests stay green. If either fails, fix or revert before continuing to the next tier — incremental builds can mask warning regressions, so re-run with `--no-incremental` if the warning count looks suspicious.
|
||||
|
||||
### Scan & verify
|
||||
|
||||
**Local `./scan-sonar.ps1` is blocked here** — the system JRE is 8 and the SonarScanner CLI requires Java 17 (`UnsupportedClassVersionError`). CI handles the scan: `.gitea/workflows/sonarqube.yml` runs `actions/setup-java@v4` with `temurin` 17 and uploads on every push and PR. So the path is **push → open PR → CI scans → re-query the API to confirm net-down**:
|
||||
|
||||
```powershell
|
||||
$h = @{ Authorization = "Basic $b64" }
|
||||
Invoke-RestMethod -Uri "$url/api/issues/search?componentKeys=sql-utilities&resolved=false&facets=rules&ps=500" -Headers $h
|
||||
```
|
||||
|
||||
The targeted rule IDs should each drop to 0 (or near-0 if the fixer left a few cases it couldn't batch-resolve). Commit subject style stays `chore(sonar): … (RuleId)` — bang (`!`) if the change is a deliberate breaking rename or visibility shift.
|
||||
|
||||
Reference in New Issue
Block a user