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

[configtls] Add NewDefault* funcs #9658

Merged

Conversation

TylerHelmuth
Copy link
Member

Description:
Adds new NewDefault* funcs for all 3 config structs.

In anticipation of the name changes from #9495 I've named the functions using the new, preferred name.

Link to tracking Issue:
Closes #9657

Testing:
Added tests

Documentation:
godoc

@TylerHelmuth TylerHelmuth requested review from a team and djaglowski February 28, 2024 02:01
Copy link

codecov bot commented Feb 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.95%. Comparing base (ae67b7c) to head (d3523ac).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9658   +/-   ##
=======================================
  Coverage   91.95%   91.95%           
=======================================
  Files         357      357           
  Lines       16501    16509    +8     
=======================================
+ Hits        15173    15181    +8     
  Misses       1000     1000           
  Partials      328      328           

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

@TylerHelmuth TylerHelmuth force-pushed the configtls-add-newdefault-funcs branch 3 times, most recently from d03acab to aa74598 Compare February 29, 2024 15:05
@@ -75,6 +75,11 @@ type Config struct {
ReloadInterval time.Duration `mapstructure:"reload_interval"`
}

// NewDefaultConfig creates a new TLSSetting with any default values set.
func NewDefaultConfig() Config {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a need for this from users perspective? Or we think it is enough only the Client/Server? Happy to keep it if we have few needs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I dont have a specific user cases, only following the pattern defined in https://github.com/open-telemetry/opentelemetry-collector/blob/main/CONTRIBUTING.md#default-configuration. The pattern implies to me that we should have a NewDefault for all public config APIs. It is probably overkill, but it does allow us to add new default values to Config in the future if we want.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather have the NewDefaultConfig function.

One thing is that we can use it in the NewDefaultClientConfig and NewDefaultServerConfig, although admittedly it doesn't need to be exported for this.

The other thing is that users can create their own configs that embed the configtls.Config struct and then they can use the configtls.NewDefaultConfig to create their instances of their configs.

I hope you don't ask me for potential examples of other configs that embed configtls.Config, because I don't have any such examples. 😄

@TylerHelmuth TylerHelmuth force-pushed the configtls-add-newdefault-funcs branch from 9921986 to 659685e Compare April 2, 2024 16:16
@TylerHelmuth TylerHelmuth requested review from mx-psi and dmitryax April 2, 2024 20:36
@codeboten codeboten merged commit 0642493 into open-telemetry:main Apr 15, 2024
49 checks passed
@TylerHelmuth TylerHelmuth deleted the configtls-add-newdefault-funcs branch April 15, 2024 17:21
@github-actions github-actions bot added this to the next release milestone Apr 15, 2024
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.

[configtls] Add NewDefault* functions for all config structs
4 participants