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

adding user configurable read timeouts #901

Merged
merged 6 commits into from
Jun 28, 2022

Conversation

joyluu-stripe
Copy link
Contributor

@joyluu-stripe joyluu-stripe commented Jun 22, 2022

Reviewers

r? @etsai-stripe
cc @stripe/developer-products

Summary

Allows user to set timeout in config file using a hidden flag (ex. stripe listen --timeout xx); if not set, defaults to 30 seconds as it was before.

Screen Shot 2022-06-27 at 12 13 34 PM

@joyluu-stripe joyluu-stripe marked this pull request as ready for review June 22, 2022 19:50
Copy link
Contributor

@etsai-stripe etsai-stripe left a comment

Choose a reason for hiding this comment

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

we can only need Timeout stored in either Config or Profile (Profile is already a field in Config). Config makes more sense imo, since Profile is more user account related values.
what we need is something like

func (c *Config) GetTimeout() int64 {
	// return customized timeout or default timeout
}

@joyluu-stripe
Copy link
Contributor Author

we can only need Timeout stored in either Config or Profile (Profile is already a field in Config). Config makes more sense imo, since Profile is more user account related values. what we need is something like

func (c *Config) GetTimeout() int64 {
	// return customized timeout or default timeout
}

gotcha, made that change! I still kept the get function in Profile since the color config seems to do the same thing (hope that's okay!) also realized that we can keep default timeout for cases where no httpClient is passed in, since that's where we pass timeout through.

Copy link
Contributor

@etsai-stripe etsai-stripe left a comment

Choose a reason for hiding this comment

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

ohh now i see why you tried to add timeout to profile. if you want to follow the way color is set on profile, lets keep everything consistent.

  1. we dont need Config.GetTimeout. we can just keep Profile.GetTimeout (sorry for the confusion earlier), but timeout field should still live only on config struct like how color does.
  2. we dont need timeout added to proxy.Config. we can pass profile.GetTimeout to the HttpClient
  3. add timeout flag to rootcmd. see example here: https://github.com/stripe/stripe-cli/blob/master/pkg/cmd/root.go#L141

@@ -263,6 +263,7 @@ func runListen(cmd *cobra.Command, address string, wg *sync.WaitGroup) error {
SkipVerify: false,
Log: logger,
NoWSS: false,
Timeout: 30,
Copy link
Contributor Author

@joyluu-stripe joyluu-stripe Jun 27, 2022

Choose a reason for hiding this comment

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

doesn't seem like there's a need to add the private flag to here actually? it seems like there's only one call to proxy.init and most parameters are getting passed default values

@@ -102,6 +102,8 @@ type Config struct {
Log *log.Logger
// Force use of unencrypted ws:// protocol instead of wss://
NoWSS bool
// Override default timeout
Timeout int64
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please explain why we need to pass this in proxy.config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, i was thinking it would be more consistent with how the other hidden flags' values are passed (like noWSS and apiBaseUrl are passed this way into proxy as well).

Copy link
Contributor

@etsai-stripe etsai-stripe 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 making all the changes!

@joyluu-stripe joyluu-stripe merged commit 8af2aaa into master Jun 28, 2022
@tomer-stripe tomer-stripe deleted the joyluu-configurable-timeout branch September 6, 2022 17:07
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.

2 participants