-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Warn on unknown environment variable; add feature gate to error out #5734
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5734 +/- ##
==========================================
+ Coverage 91.56% 91.60% +0.03%
==========================================
Files 192 192
Lines 11411 11456 +45
==========================================
+ Hits 10449 10494 +45
Misses 768 768
Partials 194 194
Continue to review full report at Codecov.
|
It looks like this needs a bigger refactor than what I originally thought :( We need to pass the logger to be able to log this warning, but for that we need to create the converter later in the process |
eddd847
to
a95eeef
Compare
This is the best I could come up with, although it comes with a lot of plumbing. The logger is not available when loading the configuration, so we need to have a way to bubble up the information that we want to be logged. To do that, I created a new @dyladan (or anyone else :)) can you think of a better solution for doing this? |
Hmm I see the issue with the logger not being constructed yet. Honestly it might be better to just fail instead of gating that behavior. The only risk is that existing collector configs might start failing on new collector versions but the collector is still 0.x so I think it isn't out of bounds to do that. Might even help users catch issues they didn't know they had. |
nonfatalerror is probably the same thing I would have come up with if you want the fail fast strategy to be gated but I might not have used the same name. The error might be fatal if there is a gate set, so it's not really the same. It's more of a sometimes fatal error than a nonfatal error. I'm trying to think of a word other than fatal which describes something that is normally not fatal but can be fatal if in a strict mode. |
Asked on #5615 for more people to participate in the discussion. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Description:
confmap.expandconverter.RaiseErrorOnUnknownEnvVar
feature gate to turn this warning into an errorLink to tracking Issue: Fixes #5615
Testing: Added unit tests