Allow environment variables in config

[bogdandrutu]

With this change values (direct values, values in maps, values in a list) that uses the expand env format see https://golang.org/pkg/os/#ExpandEnv will be expanded and users will be able to use environment variables to set these values.

Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com

[bogdandrutu]

Need more tests and polish the code. I want to see how people feel about this idea. I will finish if people like it

[pjanotti]

I like the idea. It seems better than relying on the automatic stuff that viper can do since it becomes explicit what is coming from env vars just by looking at the config file.

I would say go for it.

[tigrannajaryan]

@bogdandrutu the feature looks useful to me.

Nit: implementation-wise perhaps would be nicer to avoid using os.ExpandEnv and do string substitution manually so that it is possible to inject the values in the tests without using os.Setenv (I dislike when we have to use a global state in tests, that makes them fragile). But perhaps manual substitution too much work for now, I don't insist.

[bogdandrutu]

If only for tests I would postpone that extra work. We can make sure we don't reuse the same variable name across tests to make sure they don't interact with each other.

Also I am hearing a lot this argument against globals, but I had the impression that the testing framework will spawn a new instance for every test when run in parallel (I may be wrong).

[tigrannajaryan]

Also I am hearing a lot this argument against globals, but I had the impression that the testing framework will spawn a new instance for every test when run in parallel (I may be wrong).

I don't think Go's T.Parallel() spawns new process instances. I don't see that in the testing's source code. (I didn't dig much, so may be wrong on this).

There is also a small danger that an env variable may be accidentally defined by the calling proces and will be inherited by child go test process execution which may pollute our tests.

But, I agree, if we choose unique enough names it should not be an issue in practice.

[pjanotti]

Given that ExpandEnv is just Expand using os.GetEnv we could do unit tests using a stub instead and avoid multiple environment variables, but in the end we will need at least one of the "service" tests to use the implementation using os.GetEnv - it doesn't seem worth the trouble.

[tigrannajaryan]

@pjanotti good to know, I was not aware Expand is available separately.

[bogdandrutu]

Hitting the bug spf13/viper#673

[bogdandrutu]

@tigrannajaryan @pjanotti that is a nice option, but unfortunately our config is not an object and exposes Load with only 3 elements which means adding a new option will be a lot of unnecessary work.

I think this can be solved separately if we really want to.

[bogdandrutu]

This is now ready for review, please help me merge this :)

[codecov-io]

Codecov Report

Merging #334 into master will increase coverage by 0.17%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #334      +/-   ##
==========================================
+ Coverage   74.05%   74.22%   +0.17%     
==========================================
  Files         115      115              
  Lines        6668     6681      +13     
==========================================
+ Hits         4938     4959      +21     
+ Misses       1480     1474       -6     
+ Partials      250      248       -2

Impacted Files	Coverage Δ
config/config.go	99.2% <100%> (+0.07%)	⬆️
config/example_factories.go	53.84% <100%> (+3.02%)	⬆️
receiver/prometheusreceiver/factory.go	69.44% <0%> (-3.73%)	⬇️
receiver/prometheusreceiver/metrics_receiver.go	68.51% <0%> (+1.41%)	⬆️

---

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 96c24a3...36aa80e. Read the comment docs.

[pjanotti]

LGTM @bogdandrutu just some nits and copy and paste comment errors

[tigrannajaryan]

LGTM

[pjanotti]