-
Notifications
You must be signed in to change notification settings - Fork 8
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 VirusTotal Results to API #92
base: staging
Are you sure you want to change the base?
Add VirusTotal Results to API #92
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #92 +/- ##
===========================================
- Coverage 20.56% 19.58% -0.98%
===========================================
Files 247 256 +9
Lines 58404 61115 +2711
===========================================
- Hits 12011 11972 -39
- Misses 43863 46629 +2766
+ Partials 2530 2514 -16 ☔ View full report in Codecov by Sentry. |
|
||
func (VirustotalResult) Annotations() []schema.Annotation { | ||
return []schema.Annotation{ | ||
entsql.Annotation{Table: "virustotal_results"}, |
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.
Isn't this the same name it would be using anyway? Why override it?
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.
The generator was making the table virus_total_results
@@ -78,7 +78,7 @@ func ScanFiles(ctx context.Context, files []io.Reader, names []string) ([]ScanRe | |||
_ = errs.Wait() | |||
close(c) | |||
}() | |||
var results []ScanResult | |||
results := make([]ScanResult, 0, 100) |
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.
Why 100? Why even give a size?
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.
Saw this error in the linter,
validation/virustotal.go:81:2: Consider pre-allocating `results` (prealloc)
var results []ScanResult
^
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 think that just initializing a 0-size should be enough, setting a cap will cause problems when someone tries to upload a mod with 101 scanable items
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.
The linter is probably detecting that the channel will pass at most len(fileCount)
items, and wants you to use that as the initial size, make([]ScanResult, len(fileCount))
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.
Actually, this should probably be rewritten as a select
with a loop, because:
- We are just ignoring if the
errs
returns an error - We are not guaranteed that
fileCount
many items will be sent toc
Here is the API for the virustotal resultset from the API. First time I've ever messed with GraphQL to this extent, so I'm sure there's a lot of missing pieces to this.