Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
| vuln.Modified = fileModifiedTime | ||
| vuln.ModifiedRaw = fileModifiedTime | ||
|
|
||
| if _, err := client.Put(ctx, key, &vuln); err != nil { |
There was a problem hiding this comment.
you also need to write the vuln to GCS (and set the CustomTime field)
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new command-line tool, vuln-modified-sync, for synchronizing vulnerability modification times from local files to Datastore and GCS. The overall implementation is solid, utilizing a concurrent worker pool for efficient processing. My review focuses on improving code clarity, simplifying complex logic, and addressing a potential compatibility issue. Specifically, I've suggested making a time comparison more readable, simplifying a function that uses reflection, and replacing a Go 1.22+ specific feature with a more compatible alternative to prevent potential build failures on older Go versions.
| sliceType := reflect.SliceOf(val.Type().Elem()) | ||
| newSlice := reflect.MakeSlice(sliceType, 0, val.Len()) | ||
| hasValues := false | ||
| for i := range val.Len() { |
There was a problem hiding this comment.
The for i := range n loop over an integer n is a new feature introduced in Go 1.22. To maintain compatibility with older Go versions (pre-1.22) that the project might be using, it is safer to use the traditional for i := 0; i < val.Len(); i++ loop.
for i := 0; i < val.Len(); i++ {| } | ||
|
|
||
| // Minus 1 minute to file modified time to avoid any precision issues | ||
| if fileModifiedTime.Add(time.Minute * -1).After(vuln.Modified) { |
There was a problem hiding this comment.
The expression fileModifiedTime.Add(time.Minute * -1).After(vuln.Modified) is functionally correct, but can be written in a more readable way as fileModifiedTime.After(vuln.Modified.Add(time.Minute)). This more clearly expresses the intent of checking if the file's time is later than the datastore time plus a buffer.
Additionally, the comment on line 259 is a bit misleading. This check is more about setting a threshold to avoid updates for minor time differences, rather than a "precision issue". Consider clarifying the comment and using a named constant for the one-minute duration to improve readability.
| if fileModifiedTime.Add(time.Minute * -1).After(vuln.Modified) { | |
| if fileModifiedTime.After(vuln.Modified.Add(time.Minute)) { |
| if v != nil { | ||
| rt := reflect.ValueOf(v) | ||
| if rt.Kind() == reflect.Slice || rt.Kind() == reflect.Map { | ||
| if rt.Len() > 0 { | ||
| newMap.SetMapIndex(k, reflect.ValueOf(v)) | ||
| hasValues = true | ||
| } | ||
| } else { | ||
| newMap.SetMapIndex(k, reflect.ValueOf(v)) | ||
| hasValues = true | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic within this if v != nil block can be simplified. The recursive call to removeEmptySlicesAndMaps already ensures that v will be nil if it's an empty slice or map. Therefore, the subsequent checks for kind and length are redundant. The block can be reduced to simply setting the map index if v is not nil.
if v != nil {
newMap.SetMapIndex(k, reflect.ValueOf(v))
hasValues = true
}
| m1Strip := removeEmptySlicesAndMaps(m1) | ||
| m2Strip := removeEmptySlicesAndMaps(m2) |
There was a problem hiding this comment.
I don't think this matters in practice, but I think differences in empty slices/maps vs omitted fields should actually be considered different
|
Done. |
2 participants
Tool to update modified date back to the original value from local snapshot