fix: return error from ScanAll when every scan attempt fails - #85
fix: return error from ScanAll when every scan attempt fails#85Abhinesh Jha (Abhineshhh) wants to merge 3 commits into
Conversation
Mani Bindra (maniSbindra)
left a comment
Mani Bindra (maniSbindra)
left a comment
There was a problem hiding this comment.
detailed comment added to line 104
| } | ||
|
|
||
| for _, img := range singleImages { | ||
| stats.attempted++ |
There was a problem hiding this comment.
scanSingleImage returns nil both after a successful scan and when an existing image is skipped because --update-existing is not set. Since attempted is incremented here and failed is incremented only for a non-nil error, skipped images are counted as successful attempts.
For example, if one image is skipped and every image actually scanned fails, the counters still show a partial success and ScanAll returns nil. This appears inconsistent with the intended "all scan attempts failed" behavior.
Consider returning an explicit outcome from scanSingleImage, such as scanned/skipped/failed, and counting only images that were actually scanned. The same issue applies to stats.attempted++ at line 170.
Impact is limited because the nightly workflow passes --update-existing; this mainly affects local runs against an existing database. Non-blocking.
There was a problem hiding this comment.
You're right ,a skip returned nil and was counted as a successful attempt, so one skipped image plus a total scan outage still looked like partial success.
scanSingleImage now returns (skipped, err). scanStats.record ignores skips, so only images that were actually scanned (or tag-discovery failures) count. TestScanStatsRecord_SkipsAreNotAttempts covers the skipped + all-real-scans-failed case.
Pushed on this branch after rebase onto current main.
ScanAll logged per-image and per-repository errors but always returned nil, so a total outage still exited successfully and report generation ran against a stale database (including nightly commits). Track attempt/failure counts, keep partial-success behavior, and fail only when all attempts fail. Log a summary on partial failure. Fixes microsoft#77
scanSingleImage returns nil both on a successful scan and when an existing image is skipped without --update-existing. Counting those skips as attempts made a total outage look like partial success. Record only real scan outcomes. Skips are neither attempts nor successes. Addresses review on microsoft#85.
d3243ce to
da63a95
Compare
Add stubs for Analyze/GetTags so ScanAll can be tested without Docker. Prove a skipped existing image does not mask a total outage, and that tag-discovery and per-image failures aggregate correctly. Treat a skip paired with an error as a failed attempt.
2 participants
Description
ScanAllalways returnednileven when every repository or image failed. A total outage still exited 0, and report generation (including the nightly commit) ran against a stale database.This change tracks real scan outcomes and returns an error only when every attempt fails. Partial success still produces a report from newly scanned images. Images skipped because they already exist (without
--update-existing) are not counted as successful attempts.Related Issue
Fixes #77
Changes
scanStatsandScanAlloutcomes (including skip vs. failure)Checklist
task lintpasses locallytask testpasses locallyStacking
Independent of the Trivy/registry stacks — can merge in any order relative to them.