osv-scanner icon indicating copy to clipboard operation
osv-scanner copied to clipboard

Adding re-try for getting a Vuln for the given ID

Open davift opened this issue 2 years ago • 9 comments

davift avatar Jan 12 '23 19:01 davift

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Jan 12 '23 19:01 google-cla[bot]

I wonder if it would be better to leverage an existing library like go-retryablehttp for this - what do you think?

G-Rath avatar Jan 12 '23 19:01 G-Rath

Dear G-Rath, thanks for your contribution. It is feasible to use the mentioned library. The solution was so simple leveraging the native features already available. Do you see any drawbacks?

davift avatar Jan 12 '23 19:01 davift

While generally I'm a fan of trying to leverage native features for simple things, in this case I think the library is a better option as it has some additional features (including exponential backoff) and can just be dropped-in which means we could look to use it for other HTTP requests in future without having to do as much work (by extension, this also means it's generally going to be a better tested experience since we're not the only ones using the library).

G-Rath avatar Jan 12 '23 20:01 G-Rath

I absolutely understand your point but two things comes to my mind. First, I am dealing with a lot of SBOM at work and it is time consuming to initially inspect and later keep track of publicly known vulnerabilities such as supply chain attack. Second, give the simplicity of the feature it consumes less resources. Hope you are OK with my points.

davift avatar Jan 12 '23 20:01 davift

I'm afraid I still think that we should go with using go-retryablehttp since it is providing exactly what we want and more (that we also should want), and is well-maintained by a reputable company.

G-Rath avatar Jan 12 '23 20:01 G-Rath

Don't understand me wrong, I am not doubting the reputation of the developers of the library proposed. I just have my own opinion based on my professional experience and I don't want to increase the web if I don't think it is required. I am currently overwhelmed with code that calls opensource libraries that call more, and more, and more... I am not a coder, far from this, but as a cyber-security researcher, this would be classified as extending the attack surface (the exact opposite of what I do every day).

davift avatar Jan 12 '23 21:01 davift

While you are technically correct, it feels like you're thinking about this very one dimensionally, since there are other important factors that should be considered such as the effort involved in implementing, maintaining, testing, etc.

Ultimately though I am guest on this project so really it's @another-rex call to make - since we've now both made our opinions clear, I think we should let him weight-in before we continue to avoid rabbitholing too much 🙂

G-Rath avatar Jan 12 '23 21:01 G-Rath

While I agree in general importing in a specialize library has quite a few benefits, I think in this case because there is minimal complexity here I lean towards not importing in another external library. Especially since we currently only have two instances of http calls, both to endpoints we control.

If we need to add additional calls to other endpoints, we can bring in the go-retryablehttp library then.

another-rex avatar Jan 13 '23 01:01 another-rex

Dear another-rex,

Thanks for taking time to improve the code I proposed, it was a learning surprise. I can understand what you wrote but I could never do it myself.

Regards,

davift avatar Jan 15 '23 21:01 davift