Skip to content
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

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

Merged
merged 8 commits into from
Jan 15, 2023

Conversation

davift
Copy link
Contributor

@davift davift commented Jan 12, 2023

No description provided.

@google-cla
Copy link

google-cla bot commented Jan 12, 2023

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.

@G-Rath
Copy link
Collaborator

G-Rath commented Jan 12, 2023

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

@davift
Copy link
Contributor Author

davift commented Jan 12, 2023

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?

@G-Rath
Copy link
Collaborator

G-Rath commented Jan 12, 2023

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).

@davift
Copy link
Contributor Author

davift commented Jan 12, 2023

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.

@G-Rath
Copy link
Collaborator

G-Rath commented Jan 12, 2023

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.

@davift
Copy link
Contributor Author

davift commented Jan 12, 2023

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).

@G-Rath
Copy link
Collaborator

G-Rath commented Jan 12, 2023

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 🙂

@another-rex
Copy link
Collaborator

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.

@@ -156,7 +157,16 @@ func MakeRequest(request BatchedQuery) (*BatchedResponse, error) {

// Get a Vulnerabiltiy for the given ID.
func Get(id string) (*models.Vulnerability, error) {
resp, err := http.Get(GetEndpoint + "/" + id)
var resp *http.Response
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this logic into a separate function, that way it can be reused for the initial query above as well?

Copy link
Contributor Author

@davift davift Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! I made the mistake of renaming the branch, sorry. Learning on the fly.

@davift davift closed this Jan 13, 2023
@davift davift deleted the patch-2 branch January 13, 2023 06:32
@davift davift restored the patch-2 branch January 13, 2023 06:33
@davift davift reopened this Jan 13, 2023
Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update! Just some final suggestions to make the function a bit more general.

internal/osv/osv.go Outdated Show resolved Hide resolved
internal/osv/osv.go Outdated Show resolved Hide resolved
internal/osv/osv.go Outdated Show resolved Hide resolved
davift and others added 3 commits January 15, 2023 15:56
Co-authored-by: Rex P <106129829+another-rex@users.noreply.github.com>
Co-authored-by: Rex P <106129829+another-rex@users.noreply.github.com>
Co-authored-by: Rex P <106129829+another-rex@users.noreply.github.com>
@davift
Copy link
Contributor Author

davift commented Jan 15, 2023

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,

internal/osv/osv.go Outdated Show resolved Hide resolved
@another-rex another-rex enabled auto-merge (squash) January 15, 2023 21:22
@another-rex another-rex merged commit ad145fc into google:main Jan 15, 2023
@davift davift deleted the patch-2 branch January 15, 2023 22:45
hayleycd pushed a commit that referenced this pull request Mar 9, 2023
* Adding re-try for getting a Vuln for the given ID

* Indentation correction

* Moving the request retries to another function

* Update internal/osv/osv.go

Co-authored-by: Rex P <106129829+another-rex@users.noreply.github.com>

* Update internal/osv/osv.go

Co-authored-by: Rex P <106129829+another-rex@users.noreply.github.com>

* Update internal/osv/osv.go

Co-authored-by: Rex P <106129829+another-rex@users.noreply.github.com>

* Update internal/osv/osv.go

Co-authored-by: Rex P <106129829+another-rex@users.noreply.github.com>
julieqiu pushed a commit to julieqiu/osv-scanner that referenced this pull request May 2, 2023
* Adding re-try for getting a Vuln for the given ID

* Indentation correction

* Moving the request retries to another function

* Update internal/osv/osv.go

Co-authored-by: Rex P <106129829+another-rex@users.noreply.github.com>

* Update internal/osv/osv.go

Co-authored-by: Rex P <106129829+another-rex@users.noreply.github.com>

* Update internal/osv/osv.go

Co-authored-by: Rex P <106129829+another-rex@users.noreply.github.com>

* Update internal/osv/osv.go

Co-authored-by: Rex P <106129829+another-rex@users.noreply.github.com>
julieqiu pushed a commit to julieqiu/osv-scanner that referenced this pull request May 2, 2023
* Adding re-try for getting a Vuln for the given ID

* Indentation correction

* Moving the request retries to another function

* Update internal/osv/osv.go

Co-authored-by: Rex P <106129829+another-rex@users.noreply.github.com>

* Update internal/osv/osv.go

Co-authored-by: Rex P <106129829+another-rex@users.noreply.github.com>

* Update internal/osv/osv.go

Co-authored-by: Rex P <106129829+another-rex@users.noreply.github.com>

* Update internal/osv/osv.go

Co-authored-by: Rex P <106129829+another-rex@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants