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] newDefault for confighttp package structs #9672

Closed

Conversation

Sanket-0510
Copy link
Contributor

Description:
NewDefault methods to all the exported structs in confighttp package

Link to tracking Issue:
closes #9655

Testing:
Added test for the same.

Documentation:
godoc

@Sanket-0510 Sanket-0510 requested review from a team and jpkrohling March 2, 2024 11:23
@Sanket-0510 Sanket-0510 changed the title newDefault for confighttp package structs [confighttp] newDefault for confighttp package structs Mar 2, 2024
Copy link

codecov bot commented Mar 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.56%. Comparing base (31528ce) to head (89d1c0d).
Report is 152 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9672   +/-   ##
=======================================
  Coverage   91.56%   91.56%           
=======================================
  Files         360      360           
  Lines       16698    16704    +6     
=======================================
+ Hits        15289    15295    +6     
  Misses       1073     1073           
  Partials      336      336           

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

@Sanket-0510
Copy link
Contributor Author

why are contrib-tests failing? : |

@Sanket-0510
Copy link
Contributor Author

@astencel-sumo some contrib tests are failing in github actions, but I did ran all tests on local but nothing failed there, I also looked into the failing test it is expecting nil value for our map.

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

ClientConfig and ServerConfig depend on configtls. I think we should wait on this PR until the NewDefault functions exist for configtls.

@andrzej-stencel
Copy link
Member

ClientConfig and ServerConfig depend on configtls. I think we should wait on this PR until the NewDefault functions exist for configtls.

I don't think that's the problem. The reason the contrib tests are failing is that I asked Sanket to make the NewDefaultClientConfig and NewDefaultServerConfig to initialize the Headers and ResponseHeaders maps to empty maps. Previously those where nil. The tests in contrib expect those properties to be nil.

See e.g. here: https://github.com/open-telemetry/opentelemetry-collector/actions/runs/8231961010/job/22508354464?pr=9672#step:4:1109

=== FAIL: . TestLoadConfig/zipkin/2 (0.00s)
    config_test.go:86: 
        	Error Trace:	/tmp/opentelemetry-collector-contrib/exporter/zipkinexporter/config_test.go:86
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -39,3 +39,4 @@
        	            	   Timeout: (time.Duration) 5000000000,
        	            	-  Headers: (map[string]configopaque.String) <nil>,
        	            	+  Headers: (map[string]configopaque.String) {
        	            	+  },

I'm not exactly sure what's the preferred course of action here.

  • Allow contrib tests to fail and fix them after updating core in contrib?
  • Fix those tests before merging this change so that those tests to not check that Response and ResponseHeaders are nil?
  • ...?

What's your opinion @TylerHelmuth ?

@TylerHelmuth
Copy link
Member

Oh my comment was only a general statement, not a reason for the failing tests. I dont think we should introduce NewDefault* functions here until they can depend on the NewDefault* functions from configtls.

For the tests themselves I'd say we have 2 options:

  1. Not use empty map as the default value and instead use nil
  2. Allow contrib tests to fail and fix them after updating core in contrib

@andrzej-stencel
Copy link
Member

andrzej-stencel commented Mar 15, 2024

Oh my comment was only a general statement, not a reason for the failing tests. I dont think we should introduce NewDefault* functions here until they can depend on the NewDefault* functions from configtls.

Oh, I see 😄 Makes sense, @Sanket-0510 this means this PR should wait for #9658. After #9658 is merged, you can incorporate changes from main (by merging or rebasing) and then we can continue with this.

For the tests themselves I'd say we have 2 options:

1. Not use empty map as the default value and instead use nil

2. Allow contrib tests to fail and fix them after updating core in contrib

I don't like 1, I believe the functions should not leave the map as nil, as this would expose users of those functions to potential panic. Let's go with 2 if possible.

Copy link
Contributor

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

@TylerHelmuth
Copy link
Member

@Sanket-0510 this is ready to use the configtls. NewDefault* funcs

@Sanket-0510
Copy link
Contributor Author

@Sanket-0510 this is ready to use the configtls. NewDefault* funcs

I'll rebase!

@Sanket-0510 Sanket-0510 force-pushed the confighttp-newDefault branch 2 times, most recently from f878591 to 97499f6 Compare April 26, 2024 21:14
@Sanket-0510 Sanket-0510 force-pushed the confighttp-newDefault branch from 97499f6 to 89d1c0d Compare April 26, 2024 21:24
config/confighttp/confighttp.go Show resolved Hide resolved
config/confighttp/confighttp.go Show resolved Hide resolved
config/confighttp/confighttp.go Show resolved Hide resolved
config/confighttp/confighttp.go Show resolved Hide resolved
@andrzej-stencel andrzej-stencel self-requested a review May 6, 2024 19:34
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 May 21, 2024
Copy link
Contributor

github-actions bot commented Jun 5, 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 Jun 5, 2024
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[confighttp] Add NewDefault* functions for all config structs
3 participants