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

Error out on incomplete config sections #589

Merged
merged 7 commits into from
Aug 29, 2024

Conversation

msakrejda
Copy link
Contributor

@msakrejda msakrejda commented Aug 27, 2024

A customer ran into this because they specified some configuration settings in uppercase (DB_NAME). We currently silently skip incomplete sections, which is pretty confusing.

Separately, I considered parsing config keys in a case-insensitive manner, but since some but not all of our environment variable keys are identical to our config file keys, I thought this would be more confusing than a clear error. I'm open to revisiting that decision.

@msakrejda msakrejda requested a review from a team August 27, 2024 17:54
config/read.go Outdated Show resolved Hide resolved
@msakrejda
Copy link
Contributor Author

The reload integation test is failing; I'm not sure why, but I can reproduce it. Investigating.

@msakrejda msakrejda mentioned this pull request Aug 27, 2024
Copy link
Contributor

@keiko713 keiko713 left a comment

Choose a reason for hiding this comment

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

I think it's a good direction, but have some questions.

config/read.go Outdated Show resolved Hide resolved
config/read.go Outdated Show resolved Hide resolved
@msakrejda
Copy link
Contributor Author

This now include some other minor cleanup, in separate commits.

Copy link
Contributor

@keiko713 keiko713 left a comment

Choose a reason for hiding this comment

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

Nice, looking good 👍

config/read.go Outdated Show resolved Hide resolved
config/read.go Outdated Show resolved Hide resolved
@msakrejda msakrejda merged commit 44486e8 into main Aug 29, 2024
3 checks passed
@msakrejda msakrejda deleted the error-on-incomplete-config-sections branch August 29, 2024 17:19
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