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

[confighttp] Add read_timeout and read_header_timeout options to confighttp #10274

Closed
wants to merge 3 commits into from

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented May 31, 2024

Description

Add read_timeout and read_header_timeout options to confighttp, to use as HTTP server config options.

Link to tracking issue

Fixes #5699

Testing

Documentation

Added to README.

@atoulme atoulme requested a review from a team as a code owner May 31, 2024 04:26
@atoulme atoulme requested a review from jpkrohling May 31, 2024 04:26
Copy link

codecov bot commented May 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.38%. Comparing base (7d5b1ba) to head (6cb251a).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #10274   +/-   ##
=======================================
  Coverage   92.38%   92.38%           
=======================================
  Files         403      403           
  Lines       18729    18732    +3     
=======================================
+ Hits        17303    17306    +3     
  Misses       1066     1066           
  Partials      360      360           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@atoulme
Copy link
Contributor Author

atoulme commented May 31, 2024

This is on hold, after merging #10276

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added Stale and removed Stale labels Jun 18, 2024
Copy link
Contributor

github-actions bot commented Jul 4, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jul 4, 2024
@jpkrohling jpkrohling removed the Stale label Jul 4, 2024
@jpkrohling
Copy link
Member

@atoulme , is this still up to date?

…confighttp, to use as HTTP server config options.
@atoulme
Copy link
Contributor Author

atoulme commented Jul 19, 2024

@jpkrohling rebased and ready to go.

Handler: mux,
Addr: address,
Handler: mux,
ReadHeaderTimeout: defaultReadHeaderTimeout,
Copy link
Member

Choose a reason for hiding this comment

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

Just to double-check. this is unrelated to the change above, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's because I also removed the gosec exception. The linter picked up the fact we didn't set this on the prometheus server as well.

@jpkrohling
Copy link
Member

There are a few test failures related to this change. Can you address them?

@atoulme
Copy link
Contributor Author

atoulme commented Jul 19, 2024

There are a few test failures related to this change. Can you address them?

I am not sure how - this new config field clashes with an existing config field from contrib, and performs the same function as that field. My thinking is that we'd merge this, and update contrib and change it to use the new field. WDYT?

@jpkrohling
Copy link
Member

Right, the failures are in contrib-tests, I missed that. LGTM then.

@atoulme atoulme closed this Aug 1, 2024
@atoulme atoulme deleted the confighttp_read_timeout branch August 1, 2024 06:13
This pull request was closed.
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.

Add ReadHeaderTimeout values
3 participants