Skip to content

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, dopey and hslatman 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.

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.

}

// 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