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

Config improvements #93

Merged
merged 1 commit into from
May 20, 2024
Merged

Config improvements #93

merged 1 commit into from
May 20, 2024

Conversation

jivimberg
Copy link
Contributor

Improvements based on @sgg feedback:

  • Introducing config constructor.
  • Hiding Config fields.
  • Validating passed in configuration.
  • Making env variables win in precedence over passed in values.

…ed in configuration. Making env variables win in precedence over passed in values
@jivimberg jivimberg requested review from sgg and copperlight May 17, 2024 18:21
Copy link
Contributor

@sgg sgg left a comment

Choose a reason for hiding this comment

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

LGTM

I wonder, is there still much value in Registry.GetLogger? If not you might be able to golf that out.

func calculateCommonTags(commonTags map[string]string) map[string]string {
mergedTags := make(map[string]string)

for k, v := range commonTags {
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI there is a maps package that provides some niceties for cloning/copying values, though it doesn't filter the way you're doing here https://pkg.go.dev/golang.org/x/exp/maps

@jivimberg
Copy link
Contributor Author

LGTM

I wonder, is there still much value in Registry.GetLogger? If not you might be able to golf that out.

I'd seen usages of this so I prefer keeping it.

@jivimberg jivimberg merged commit d5e58de into main May 20, 2024
2 checks passed
@jivimberg jivimberg deleted the juan/config-improvements branch May 20, 2024 19:10
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.

3 participants