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 confighttp.NewDefaultServerConfig() #10275

Merged
merged 4 commits into from
Jul 22, 2024

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented May 31, 2024

Description

add a new default method to instantiate the HTTP server config.

Link to tracking issue

#9655

@atoulme atoulme requested review from a team and bogdandrutu May 31, 2024 05:01
Copy link

codecov bot commented May 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.28%. Comparing base (534768c) to head (c3cbda7).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #10275   +/-   ##
=======================================
  Coverage   92.27%   92.28%           
=======================================
  Files         397      397           
  Lines       18723    18736   +13     
=======================================
+ Hits        17277    17290   +13     
  Misses       1086     1086           
  Partials      360      360           

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

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 the Stale label Jun 19, 2024
@atoulme atoulme removed the Stale label Jun 22, 2024
@atoulme atoulme force-pushed the newdefaultserverconfig branch 2 times, most recently from b7852d4 to fe60e85 Compare June 25, 2024 21:06
…alize structs as part of the default server config
@jpkrohling
Copy link
Member

I discussed this in person with @atoulme yesterday, and I missed the fact that we don't currently expose the timeouts to our end-users. I'm OK with this PR as is, but would prefer to have that option exposed first, and a default being set here, so that we have fewer breaking changes.

@atoulme
Copy link
Contributor Author

atoulme commented Jun 27, 2024

If we add the timeout fields, they take over/conflict with the fields declared for one receiver in contrib. I think we could skip the test in contrib, merge this, fix in contrib. WDYT?

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 the Stale label Jul 12, 2024
@atoulme atoulme removed the Stale label Jul 15, 2024
@mx-psi
Copy link
Member

mx-psi commented Jul 18, 2024

What's the status of this one?

@atoulme
Copy link
Contributor Author

atoulme commented Jul 20, 2024

What's the status of this one?

It's ready to merge, if we are ok with breaking the build with contrib, or we can merge open-telemetry/opentelemetry-collector-contrib#34181 to get a passing build.

jpkrohling pushed a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request Jul 20, 2024
See open-telemetry/opentelemetry-collector#10275
for context - we need to disable a test so we can merge the change in
core, and bring it here next.
@jpkrohling
Copy link
Member

I merged the contrib PR.

@atoulme
Copy link
Contributor Author

atoulme commented Jul 20, 2024

@jpkrohling thanks the tests now pass.

@mx-psi mx-psi merged commit 0e14c22 into open-telemetry:main Jul 22, 2024
50 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 22, 2024
@atoulme atoulme deleted the newdefaultserverconfig branch July 23, 2024 18:55
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.

5 participants