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.

Sign up for GitHub

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

Jump to bottom

change api http.client to interface #1387

Merged
merged 1 commit into from
Dec 20, 2023
Merged

change api http.client to interface #1387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 10 additions & 6 deletions api/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,18 @@ var DefaultRoundTripper http.RoundTripper = &http.Transport{
TLSHandshakeTimeout: 10 * time.Second,
}

type HttpClient interface {
Copy link
Member

Choose a reason for hiding this comment

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

Why not using simply http.RoundTripper?

Copy link
Member

Choose a reason for hiding this comment

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

Also if anything this is not following style guide (should be HTTPClient) and does not have commentary as public interface

Copy link
Member

Choose a reason for hiding this comment

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

It's bit different method name, but still.. RoundTrip(*Request) (*Response, error)

In theory the api/http packages are experimental so we can change them with breaking changes.

Copy link
Member

Choose a reason for hiding this comment

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

But even with all of this the use case mentioned was possible without this change I believe:

There is even documentation how to do it here: github.com/hashicorp/go-retryablehttp/Client

Copy link
Author

@tsipo tsipo Dec 22, 2023 •
edited

Choose a reason for hiding this comment

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

Hi @bwplotka , thanks for your comments.

Your suggested solution (sorry, somehow I missed that in hashicorp's docs) to use their StandardClient() converter to the library's http.Client works - I have just tried that out and saw retries. So the use-case is possible without this change.

In view of that, you can decide. I still don't think that adding the interface is a bad idea as this is what the Prometheus client requires (only the Do() function). It's true that hashicorp's client gives you a converter to the standard client, but you are now relying on that. I'll leave it up to you.

Do(req *http.Request) (*http.Response, error)
}

// Config defines configuration parameters for a new client.
type Config struct {
// The address of the Prometheus to connect to.
Address string

// Client is used by the Client to drive HTTP requests. If not provided,
// a new one based on the provided RoundTripper (or DefaultRoundTripper) will be used.
Client *http.Client
// a new http.Client based on the provided RoundTripper (or DefaultRoundTripper) will be used.
Client HttpClient

// RoundTripper is used by the Client to drive HTTP requests. If not
// provided, DefaultRoundTripper will be used.
Expand All @@ -57,13 +61,13 @@ func (cfg *Config) roundTripper() http.RoundTripper {
return cfg.RoundTripper
}

func (cfg *Config) client() http.Client {
func (cfg *Config) client() HttpClient {
tsipo marked this conversation as resolved.
Show resolved Hide resolved
if cfg.Client == nil {
return http.Client{
return &http.Client{
Transport: cfg.roundTripper(),
}
}
return *cfg.Client
return cfg.Client
}

func (cfg *Config) validate() error {
Expand Down Expand Up @@ -101,7 +105,7 @@ func NewClient(cfg Config) (Client, error) {

type httpClient struct {
endpoint *url.URL
client http.Client
client HttpClient
}

func (c *httpClient) URL(ep string, args map[string]string) *url.URL {
Expand Down
2 changes: 1 addition & 1 deletion api/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func TestClientURL(t *testing.T) {

hclient := &httpClient{
endpoint: ep,
client: http.Client{Transport: DefaultRoundTripper},
client: &http.Client{Transport: DefaultRoundTripper},
}

u := hclient.URL(test.endpoint, test.args)
Expand Down