Skip to content

[feat] Add retries for HTTP operations#218

Merged
radeksimko merged 6 commits intohashicorp:mainfrom
james0209:retryable-http
May 28, 2024
Merged

[feat] Add retries for HTTP operations#218
radeksimko merged 6 commits intohashicorp:mainfrom
james0209:retryable-http

Conversation

@james0209
Copy link
Copy Markdown
Contributor

@james0209 james0209 commented May 25, 2024

What

Use go-retryablehttp for retryable network operations.

Why

closes #140

@james0209 james0209 requested a review from a team as a code owner May 25, 2024 12:44
Copy link
Copy Markdown
Member

@radeksimko radeksimko 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 PR.

Could we keep the HTTP client logic in that httpclient package and also retain the logic which adds the User-Agent header?

For example:

// NewHTTPClient provides a pre-configured http.Client
// e.g. with relevant User-Agent header
func NewHTTPClient() *http.Client {
	client := retryablehttp.NewClient().StandardClient()
	client.Transport = &userAgentRoundTripper{
		userAgent: fmt.Sprintf("hc-install/%s", version.Version()),
		inner:     client.Transport,
	}
	return client
}

@james0209
Copy link
Copy Markdown
Contributor Author

james0209 commented May 28, 2024

Thanks for the PR.

Could we keep the HTTP client logic in that httpclient package and also retain the logic which adds the User-Agent header?

For example:

// NewHTTPClient provides a pre-configured http.Client
// e.g. with relevant User-Agent header
func NewHTTPClient() *http.Client {
	client := retryablehttp.NewClient().StandardClient()
	client.Transport = &userAgentRoundTripper{
		userAgent: fmt.Sprintf("hc-install/%s", version.Version()),
		inner:     client.Transport,
	}
	return client
}

@radeksimko I've got no problem putting it in the httpclient 🙂

Although from what I can tell, the UserAgent is not used currently. It is set on something called cli, but then only the client is returned and used. UserAgent is only used to be assigned to cli but cli is never returned or used anywhere - only client.

// NewHTTPClient provides a pre-configured http.Client
// e.g. with relevant User-Agent header
func NewHTTPClient() *http.Client {
	client := cleanhttp.DefaultClient()

	userAgent := fmt.Sprintf("hc-install/%s", version.Version())

	cli := cleanhttp.DefaultPooledClient()
	cli.Transport = &userAgentRoundTripper{
		userAgent: userAgent,
		inner:     cli.Transport,
	}

	return client
}

I could do something like you suggested above if you would like UserAgent to actually be utilized?

@radeksimko
Copy link
Copy Markdown
Member

Although from what I can tell, the UserAgent is not used currently.

Yes, I noticed it isn't used - that was certainly not intentional - i.e. that's a bug that would be worth addressing also.

I could do something like you suggested above if you would like UserAgent to actually be utilized?

Yes please.

@james0209 james0209 requested a review from radeksimko May 28, 2024 14:58
Copy link
Copy Markdown
Member

@radeksimko radeksimko 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.

Proposal: Add retries to network operations

2 participants