Using httputil for NVD #610
Conversation
ext/vulnmdsrc/nvd/nvd.go
Outdated
| if err != nil { | ||
| log.WithError(err).WithField(logDataFeedName, dataFeedName).Error("could not download NVD data feed") | ||
| return dataFeedReaders, dataFeedHashes, commonerr.ErrCouldNotDownload | ||
| } | ||
| // r is closed in BuildCache() |
ext/vulnmdsrc/nvd/nvd.go
Outdated
| if err != nil { | ||
| return "", err | ||
| } | ||
| defer r.Body.Close() | ||
|
|
||
| if !httputil.Status2xx(r) { | ||
| return "", errors.New("Unsuccesuful status code: " + string(r.StatusCode)) |
There was a problem hiding this comment.
Unsuccessful
Also having some more context in this message would be nice. I have no idea what request was being tried that returned a non-200 status code.
|
I was wrong at first with that comment, because that's where it crashed when I had defer there. |
ext/vulnmdsrc/nvd/nvd.go
Outdated
| @@ -171,7 +171,7 @@ func getDataFeeds(dataFeedHashes map[string]string, localPath string) (map[strin | |||
| log.WithError(err).WithField(logDataFeedName, dataFeedName).Error("could not download NVD data feed") | |||
| return dataFeedReaders, dataFeedHashes, commonerr.ErrCouldNotDownload | |||
| } | |||
| // r is closed in BuildCache() | |||
| defer r.Body.Close() | |||
There was a problem hiding this comment.
This won't close anything until the whole function getDataFeeds() exits. This is not what we want to do.
It could end up being more trouble than it's worth, but wrapping everything inside the loop iteration into a it's own function might be the best way to get defer to execute at the right time.
There was a problem hiding this comment.
Yeah, it was a little outside of what I set out with my initial PR, but I agree with you. I think I wrapped it up nicely now. Without spending too much effort.
ext/vulnmdsrc/nvd/nvd.go
Outdated
| log.WithError(err).WithField(logDataFeedName, dataFeedName).Error("could not download NVD data feed") | ||
| return dataFeedReaders, dataFeedHashes, commonerr.ErrCouldNotDownload | ||
| } | ||
| downloadAndSave := func() error { |
There was a problem hiding this comment.
I think you should pull this out into a top-level function called downloadFeed, rather than an inline function. After that LGTM.
MackJM
force-pushed
the
wip/master_nvd_httputil
branch
from
cd0db72
to
fecdf48
Compare
ext/vulnmdsrc/nvd/nvd.go
Outdated
| // Store it to a file at the same time if possible. | ||
| f, err := os.Create(fileName) | ||
| if err != nil { | ||
|
|
There was a problem hiding this comment.
Sorry for the OCD, but drop this newline and we're good to go.
nvd was missed when moving to httputil, this fixes it
…elp with resource management
MackJM
force-pushed
the
wip/master_nvd_httputil
branch
from
fecdf48
to
30848d9
Compare
jzelinskie
added
kind/cleanup
nvd was missed when moving to httputil, this fixes it