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

Warn on unknown environment variable; add feature gate to error out #5734

Closed

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Jul 22, 2022

Description:

  • Add warning when expanding unknown environment variable.
  • Add confmap.expandconverter.RaiseErrorOnUnknownEnvVar feature gate to turn this warning into an error

Link to tracking Issue: Fixes #5615

Testing: Added unit tests

@mx-psi mx-psi marked this pull request as ready for review July 22, 2022 22:52
@mx-psi mx-psi requested a review from a team as a code owner July 22, 2022 22:52
@mx-psi mx-psi requested review from dmitryax and dyladan July 22, 2022 22:52
@codecov
Copy link

codecov bot commented Jul 22, 2022

Codecov Report

Merging #5734 (eddd847) into main (3e14ee9) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head eddd847 differs from pull request most recent head a95eeef. Consider uploading reports for the commit a95eeef to get more accurate results

@@            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              
Impacted Files Coverage Δ
confmap/converter/expandconverter/expand.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e14ee9...a95eeef. Read the comment docs.

@mx-psi mx-psi marked this pull request as draft July 22, 2022 23:23
@mx-psi
Copy link
Member Author

mx-psi commented Jul 22, 2022

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

@mx-psi mx-psi force-pushed the mx-psi/warn-on-unknown-envvar branch from eddd847 to a95eeef Compare July 23, 2022 11:24
@mx-psi
Copy link
Member Author

mx-psi commented Jul 23, 2022

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 nonfatalerror package to signal that an error is not fatal.

@dyladan (or anyone else :)) can you think of a better solution for doing this?

@dyladan
Copy link
Member

dyladan commented Aug 4, 2022

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.

@dyladan
Copy link
Member

dyladan commented Aug 4, 2022

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.

@mx-psi
Copy link
Member Author

mx-psi commented Aug 5, 2022

Asked on #5615 for more people to participate in the discussion.

@github-actions
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 the Stale label Aug 20, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 3, 2022

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

@github-actions github-actions bot closed this Sep 3, 2022
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.

Log a warning when an environment variable without a value is expanded
2 participants