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

Update pkg/osv to allow overriding the http client / transport. #357

Merged
merged 1 commit into from
May 1, 2023

Conversation

jeffmendoza
Copy link
Contributor

@jeffmendoza jeffmendoza commented Apr 25, 2023

I don't know if you all intended pkg/osv to be a general purpose https://osv.dev/ client library, but we are using it as that here: https://github.com/guacsec/guac/blob/main/pkg/certifier/osv/osv.go

It would be nice to override the http client used to insert custom transports, etc.

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.

Being able to override the client seems like a good feature to have.

The main concern with this solution is if two libraries are both using the osv package, (if I understand go packages correctly), and one overrides the HTTPClient this will change the client for all libraries using osv.go, which might not be desirable.

A potentially better solution could be to create two separate functions (e.g. GetWithClient, HydrateWithClient) which takes in a http client as well, and move the logic in there. Then just make the original Get and Hydrate functions wrap the new functions, filling the client argument with http.DefaultClient.

@jeffmendoza
Copy link
Contributor Author

Yes, that is correct. If it were used in two places in the same binary, there could be contention. I was looking to be the least disruptive.

Updated with the "WithClient" strategy.

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.

LGTM!

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.

2 participants