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

[feat] Add retries for HTTP operations #218

Merged
merged 6 commits into from
May 28, 2024
Merged

[feat] Add retries for HTTP operations #218

merged 6 commits into from
May 28, 2024

Conversation

james0209
Copy link
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
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
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
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
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

@radeksimko radeksimko merged commit b439987 into hashicorp:main May 28, 2024
11 checks passed
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