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

Transport wrappers #2103

Merged
merged 3 commits into from
Dec 12, 2024
Merged

Transport wrappers #2103

merged 3 commits into from
Dec 12, 2024

Conversation

azazeal
Copy link
Contributor

@azazeal azazeal commented Dec 12, 2024

This PR adds references to a transport wrapper that callers may pass in order to inject their own dependecies to references of HTTP clients that the internal packages generate.

@github-actions github-actions bot added the needs triage Waiting for discussion / prioritization by team label Dec 12, 2024
@azazeal azazeal removed the needs triage Waiting for discussion / prioritization by team label Dec 12, 2024
@azazeal azazeal changed the title WIP: Transport wrappers Transport wrappers Dec 12, 2024
@azazeal azazeal requested review from areed, hslatman and dopey December 12, 2024 14:52
@azazeal azazeal marked this pull request as ready for review December 12, 2024 14:52
areed
areed previously approved these changes Dec 12, 2024
func (w *Webhook) DoWithContext(ctx context.Context, client *http.Client, reqBody *webhook.RequestBody, data any) (*webhook.ResponseBody, error) {
// TransportWrapper wraps the set of functions mapping [http.Transport] references to
// [http.RoundTripper].
type TransportWrapper = httptransport.Wrapper
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The httptransport package is an internal one, to avoid duplication from package to package. Since the function's signature is inside function definitions I thought to compress the signature by declaring a type alias.

@@ -264,6 +264,9 @@ type Config struct {
// HTTPClient is an HTTP client that trusts the system cert pool and the CA
// roots.
HTTPClient *http.Client
// WrapTransport references the function that should wrap any [http.Transport] initialized
// down the Config's chain.
WrapTransport func(*http.Transport) http.RoundTripper
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not httptransport.Wrapper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it'll be a type alias but it's coming right up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -103,6 +104,22 @@ func WithWebhookClient(c *http.Client) Option {
}
}

// Wrapper wraps the set of functions mapping [http.Transport] references to [http.RoundTripper].
type TransportWrapper = httptransport.Wrapper
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above the above.

@azazeal azazeal requested a review from areed December 12, 2024 17:28
@azazeal azazeal enabled auto-merge (squash) December 12, 2024 17:50
@azazeal azazeal merged commit 809c702 into master Dec 12, 2024
13 checks passed
@azazeal azazeal deleted the panos/transport branch December 12, 2024 17:51
@hslatman hslatman added this to the v0.28.2 milestone Dec 12, 2024
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.

4 participants