Use environment variables when error is strict #731
Conversation
** Summary ** Fix minor bug in readConfig function when cheking the error of unmarshalSemiStrictly. ** Motivation ** This will allow us to use a configuration that has an unmarshalling strict error, i.e. when the configuration have unknown fields, and the validate-config and validate-config-strict command-line flags are both false. ** Test plan ** Automated testing. ** Rollout/monitoring/revert plan ** None
lahiguera
force-pushed
the
read_config_environment_variables
branch
from
623023e to
ede34b6
Compare
Gerald Rule: Copy Observability on Veneur, Unilog, Falconer pull requestscc @stripe/observability |
There was a problem hiding this comment.
I think this looks good (and good catch on the error variable mismatch!) but I'm a little worried about the env vars. Cc @aditya-stripe/@ChimeraCoder to make sure I'm worrying over nothing (:
| @@ -135,7 +135,7 @@ func readConfig(r io.Reader) (Config, error) { | |||
| } | |||
| unmarshalErr := unmarshalSemiStrictly(bts, &c) | |||
| if unmarshalErr != nil { | |||
| if _, ok := err.(*UnknownConfigKeys); !ok { | |||
| if _, ok := unmarshalErr.(*UnknownConfigKeys); !ok { | |||
There was a problem hiding this comment.
oufff, that's a good catch!
| @@ -37,6 +37,18 @@ func TestReadBadConfig(t *testing.T) { | |||
| assert.Equal(t, c, Config{}, "Parsing invalid config file should return zero struct") | |||
| } | |||
|
|
|||
| func TestReadBadConfig_EnvVarsAreNotRead(t *testing.T) { | |||
| os.Setenv("VENEUR_HOSTNAME", "cux") | |||
| defer os.Unsetenv("VENEUR_HOSTNAME") | |||
There was a problem hiding this comment.
Hm, I'm mildly worried about setting and unsetting environment variables in tests that could run parallel with other things (even though you don't use t.Parallel() here) - couldn't find envconfig testing recommendations either, but I guess it's probably fine as long as we never parallelize.
Cc @aditya-stripe for a second opinion
There was a problem hiding this comment.
Thanks for reviewing this!
Environment variables are in effect global variables, and these IMO should be accessed only from the most exterior layer of the app, and then passed as arguments to more interior layers. Having a package with "constants" might be helpful if we don't want to "pollute" the signatures of the functions. In Go I know this cannot be enforced at compile time since consts are only for values known at compile time, but an alternative would be having functions instead that return the desired environment variables values. It's just an idea :)
asf-stripe
requested review from
aditya-stripe
and removed request for
asf-stripe
|
Handing this over to aditya! (: |
|
|
Summary
Fix minor bug in
readConfigfunction when checking the error ofunmarshalSemiStrictly.Motivation
This will allow us to use a configuration that has an unmarshalling
strict error, i.e. when the configuration have unknown fields, and
the
validate-configandvalidate-config-strictcommand-lineflags are both false.
Test plan
Automated testing.
Rollout/monitoring/revert plan
None