-
Notifications
You must be signed in to change notification settings - Fork 355
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
Add experimental-download-offline-databases
flag
#1039
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is cool!
I'm often downloading one or a few ecosystems for offline validation or other examination purposes, and have this shell script I had Gemini cook up for me to make this streamlined.
Is the intention for this feature to be used independently of a scanning run, or as part of one?
Would it be possible to include another flag that accepted a subset of ecosystems to download, in the interests of saving time, bandwidth and disk space for this purpose?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1039 +/- ##
==========================================
+ Coverage 65.23% 65.29% +0.05%
==========================================
Files 150 150
Lines 12497 12498 +1
==========================================
+ Hits 8153 8160 +7
+ Misses 3882 3878 -4
+ Partials 462 460 -2 ☔ View full report in Codecov by Sentry. |
For now, this flag is only valid when I think ideally, we can have some subcommand for example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, but the flag should be plural because there are many databases and really the number of them is an implementation detail (right now its 1:1 with ecosystems, but maybe in future they'll be sharded differently 🤷)
I also wonder if the flag should be something like --download-offline-databases
, to tie it more to the --offline
flag 🤔
internal/local/zip.go
Outdated
@@ -27,8 +27,8 @@ type ZipDB struct { | |||
Name string | |||
// the url that the zip archive was downloaded from | |||
ArchiveURL string | |||
// whether this database should make any network requests | |||
Offline bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw I think it would be better to keep the existing structure within the database logic, as it's technically possible they could be implemented to make network requests before downloading and (more practically) it means a smaller diff + aligns better with everything else (e.g. the error is ErrOfflineDatabaseNotFound
, the tests say "if the db is offline", etc).
The fact that we're exposing this to the user as "download databases" is something of an implementation detail of the CLI.
Having said that, I won't block if you can't be bothered reverting 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just feel it's weird to use downloadDB
to indicate offline
so would like to keep consistent in the names, but I don't mind too much keeping offline
in ZipDB
.
@andrewpollock I proposed basically this exact thing in subcommand form at our weekly catchup 😄 My thinking is especially now that we have both subcommands and interactive-ness, we could provide a very nice interface for managing these databases and their location without overloading Part of me now wonders if it could be worth having a dedicated |
experimental-download-database
flagexperimental-offline-download-databases
flag
I personally think |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just think we should swap offline and download around in the flag.
@G-Rath do you mind taking another look? Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! got a minor comment about the doc wording, but it's optional :)
experimental-offline-download-databases
flagexperimental-download-offline-databases
flag
Currently flags
experimental-offline
andexperimental-local-db
are confusing sometimes.This PR renames
experimental-local-db
toexperimental-download-database
to make it more explicit whether to download the database or not.For now,
experimental-download-database
only works whenexperimental-offline
is set.internal/local
is also modified to reflect the change in the naming and meaning of this flag.