Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(offline): report all ecosystems without local databases in one single line #1279

Merged
merged 2 commits into from
Sep 30, 2024

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Sep 25, 2024

This reduces the amount of noise generated when using --offline if a database is not available for a particular ecosystem. While it was suggested that we could have some form of general error cache, for now I've just optimized for this specific case as really it's the one we expect the most and (in part) I think the nature of Go makes the more generic improvement a bit too faffy vs the gain.

Resolves #1005

@@ -641,6 +641,35 @@ func TestRun_LocalDatabases(t *testing.T) {
}
}

func TestRun_LocalDatabases_AlwaysOffline(t *testing.T) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I've had to put this in a dedicated test because our other local db tests try to reuse the database as an optimization when run locally, which is exactly what we don't want here

@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 4 lines in your changes missing coverage. Please review.

Project coverage is 68.44%. Comparing base (ce76735) to head (c04e4c3).

Files with missing lines Patch % Lines
internal/local/check.go 71.42% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1279      +/-   ##
==========================================
+ Coverage   68.37%   68.44%   +0.06%     
==========================================
  Files         175      175              
  Lines       16786    16798      +12     
==========================================
+ Hits        11478    11497      +19     
+ Misses       4677     4672       -5     
+ Partials      631      629       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +154 to +155
// the most likely error at this point is that the PURL could not be parsed
r.Errorf("could not load db for %s ecosystem: %v\n", pkg.Ecosystem, err)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: this no longer has coverage, but that's fine

while my comment mentions a possible error being a PURL could not be parsed, we've not got a test for that and I'm not sure how easy it'll be - I've made a note to look into that as a separate thing, but we shouldn't let it block this as I think we want to be playing it safe i.e. by logging the error rather than assume the only possible error could be that the database cannot be found

@another-rex another-rex merged commit 866b3e0 into google:main Sep 30, 2024
13 checks passed
@another-rex another-rex deleted the improve-offline-errors branch September 30, 2024 01:59
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.

Errors spamming the stderr output
4 participants