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

Increase slash command timeout to 30 seconds #375

Merged
merged 1 commit into from
May 9, 2023

Conversation

mickmister
Copy link
Member

Summary

The timeout for slash commands is currently 6 seconds, which is causing issues for the /gitlab webhook add command. This PR increases the timeout to 30 seconds, which is what is currently used for incoming http requests:

requestTimeout = 30 * time.Second

Ticket Link

Fixes #374

@mickmister mickmister added the 2: Dev Review Requires review by a core committer label May 3, 2023
@mickmister mickmister requested a review from hanzei May 3, 2023 19:41
@mickmister mickmister requested a review from spirosoik as a code owner May 3, 2023 19:41
@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (de004a9) 32.20% compared to head (2d8303d) 32.20%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #375   +/-   ##
=======================================
  Coverage   32.20%   32.20%           
=======================================
  Files          21       21           
  Lines        3465     3465           
=======================================
  Hits         1116     1116           
  Misses       2242     2242           
  Partials      107      107           
Impacted Files Coverage Δ
server/command.go 23.60% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -88,7 +88,7 @@ const (
)

const (
commandTimeout = 6 * time.Second
commandTimeout = 30 * time.Second
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't be this configurable instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

@spirosoik In general our solution has been to increase this to 30 seconds in plugin projects. I think having this as a plugin config setting is surfacing an implementation detail, but I'm not opposed to having it configurable. We would want to support configuring the http timeout as well, and be consistent on other projects and apply it there too. What do you think?

@mickmister
Copy link
Member Author

We'll defer QA, from offline discussion with @DHaussermann

@mickmister mickmister added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels May 9, 2023
@mickmister mickmister merged commit c3e7bbc into master May 9, 2023
11 checks passed
@mickmister mickmister deleted the increase-context-timeout branch May 9, 2023 15:28
@mickmister mickmister mentioned this pull request May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Context deadline exceeded" for webhook add command
3 participants