Support env substitution in configuration file if variable not set #4458

neelayu · 2021-11-18T16:06:11Z

Description:
FEAT: Support for expanding environment variable in case it is not set.
The basic syntax is:
${ENV_VAR:-DEFAULT_VALUE}

Link to tracking Issue: #4384

Testing: Unit Tests
This PR is similar to #4387, instead this doesn't use extra dependencies.

I think this is a relatively common ask especially when configuration is allowed via environment variables. A fallback is expected in case the variable is not set.

codecov · 2021-11-18T16:13:09Z

Codecov Report

Merging #4458 (358688f) into main (db4aa87) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4458      +/-   ##
==========================================
+ Coverage   90.69%   90.70%   +0.01%     
==========================================
  Files         178      178              
  Lines       10356    10361       +5     
==========================================
+ Hits         9392     9398       +6     
+ Misses        746      745       -1     
  Partials      218      218

Impacted Files	Coverage Δ
config/configmapprovider/expand.go	`73.68% <100.00%> (+1.98%)`	⬆️
exporter/otlpexporter/otlp.go	`72.83% <0.00%> (ø)`
client/client.go	`89.65% <0.00%> (+3.94%)`	⬆️

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 db4aa87...358688f. Read the comment docs.

mx-psi · 2021-11-18T16:29:51Z

config/configmapprovider/expand.go

@@ -86,6 +87,10 @@ func expandEnv(s string) string {
 		if str == "$" {
 			return "$"
 		}
-		return os.Getenv(str)
+		pair := strings.Split(str, ":")
+		if envValue := os.Getenv(pair[0]); envValue != "" || len(pair) == 1 {


Getenv doesn't distinguish between an empty value and an unset variable, we probably want to use LookupEnv instead, so that people can set an empty value if needed

ACK. Changed to LookupEnv

mx-psi

This needs a release note on the changelog

mx-psi · 2021-11-19T08:50:46Z

config/configmapprovider/testdata/expand-with-all-env.yaml

@@ -1,5 +1,12 @@
 test_map:
  extra: "$EXTRA"
+  env:
+    recv.1: "$ENV_VALUE_1"
+    recv.2: "${ENV_VALUE_2:default value for 2 not substituted}"


Bash and zsh use the ${parameter:-word} syntax (${name-word} works too on zsh). I would prefer to stick to the known convention for this.

Noted. I have changed the syntax to ${parameter:-word}

mx-psi · 2021-11-19T08:53:54Z

config/configmapprovider/expand.go

@@ -86,6 +87,10 @@ func expandEnv(s string) string {
 		if str == "$" {
 			return "$"
 		}
-		return os.Getenv(str)
+		pair := strings.Split(str, ":")


On ${ENV:a:b} if ENV is unset it will be replaced by a instead of a:b. You need to use SplitN instead with n equals 1 2, to split only by the first separator :. I would add this as a testcase too.

Good catch. Now using SplitN

mx-psi

This looks good to me and I believe it's easier to accept for maintainers since it doesn't rely on an external dependency and the logic is simple enough. I left a small comment about the changelog wording

CHANGELOG.md

Co-authored-by: Pablo Baeyens <[email protected]>

bogdandrutu · 2021-11-19T17:24:39Z

@mx-psi thanks for the review :) but if you want to make everything nice and consistent I want you to think also about:

So a TL;DR users asks us to expend more than just envvar and we need to have a "syntax" consistent for all these sources.

tigrannajaryan

I am currently working on the first part of #4190 (config_sources). After that I will work on value_source support, which is a more generalized case of env variable substitution. Until that is done it is best not to add further features to env variables otherwise we may end up with env variables being unsupportable special case. I am blocking this to avoid that.

mx-psi · 2021-11-22T11:05:15Z

Thanks for the links @bogdandrutu and @tigrannajaryan, it's useful context. I don't have the full context on the final look of config sources, so I wasn't aware about this interfering with these efforts.

It may be useful for contributors to point to this on the issue linked (and if config sources work makes certain parts of the Collector to be on a "feature freeze" phase, it may be useful to explicitly document this somewhere too).

tigrannajaryan · 2021-11-29T18:48:59Z

Update: I am not actively looking into this right now. @bogdandrutu will probably take it over. I will remove by block so that you don't need to wait for me.

Not working on this anymore.

github-actions · 2021-12-07T03:15:58Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

github-actions · 2021-12-16T03:15:56Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

github-actions · 2021-12-31T03:16:05Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

bogdandrutu · 2022-01-13T17:33:59Z

CHANGELOG.md

@@ -6,6 +6,8 @@

 - Remove `pdata.AttributeMap.InitFromMap` (#4429)

+## 💡 Enhancements 💡
+- Support shell-like syntax for environment variable substitution defaults (`${ENV:-default}`) (#4458)


Food for thought: We are going towards a direction of using "URI" like locations/identifiers for configs. I do understand that this is natural for lots of the people, but we should consider something like "env:ENV?default=foo" or in this direction.

It feels unnatural to have $ENV when there is no default and $env:ENV?default=foo when there is a default; is that what we would be aiming for?

I am proposing ${env:ENV} and ${env:ENV?default=foo}

That seems acceptable to me, it's close to PowerShell's syntax so it's not completely new. It will be a big breaking change, for end-users, though.

I think for few versions (4-5) we will still support the old way. We are not going to just break the users :)

Sounds like a sensible plan to me!

github-actions · 2022-02-04T03:15:20Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions · 2022-02-18T03:15:55Z

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

ringerc · 2023-11-14T19:33:48Z

See #2534 (comment) for a workaround

Support env substitution without dependency

a322225

neelayu requested review from a team and dmitryax November 18, 2021 16:06

mx-psi reviewed Nov 18, 2021

View reviewed changes

Use LookupEnv instead of GetEnv

760d341

mx-psi requested changes Nov 19, 2021

View reviewed changes

Address review comments, Change syntax and add changelog entry

5f5c5be

mx-psi approved these changes Nov 19, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

358688f

Co-authored-by: Pablo Baeyens <[email protected]>

tigrannajaryan previously requested changes Nov 19, 2021

View reviewed changes

github-actions bot added the Stale label Dec 7, 2021

bogdandrutu removed the Stale label Dec 8, 2021

github-actions bot added the Stale label Dec 16, 2021

bogdandrutu removed the Stale label Dec 16, 2021

github-actions bot added the Stale label Dec 31, 2021

bogdandrutu removed the Stale label Jan 3, 2022

neelayu mentioned this pull request Jan 13, 2022

Move expand config map converter back out of internal package #4672

Merged

bogdandrutu reviewed Jan 13, 2022

View reviewed changes

github-actions bot added the Stale label Feb 4, 2022

github-actions bot closed this Feb 18, 2022

neelayu mentioned this pull request May 20, 2022

Support default values for configuration environment variables #5228

Closed

Support env substitution in configuration file if variable not set #4458

Support env substitution in configuration file if variable not set #4458

Uh oh!

Conversation

neelayu commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mx-psi left a comment

Choose a reason for hiding this comment

Uh oh!

mx-psi Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mx-psi Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mx-psi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bogdandrutu commented Nov 19, 2021

Uh oh!

tigrannajaryan left a comment

Choose a reason for hiding this comment

Uh oh!

mx-psi commented Nov 22, 2021

Uh oh!

tigrannajaryan commented Nov 29, 2021

Uh oh!

github-actions bot commented Dec 7, 2021

Uh oh!

github-actions bot commented Dec 16, 2021

Uh oh!

github-actions bot commented Dec 31, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogdandrutu Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 4, 2022

Uh oh!

github-actions bot commented Feb 18, 2022

Uh oh!

ringerc commented Nov 14, 2023

Uh oh!

Uh oh!

neelayu commented Nov 18, 2021 •

edited

Loading

codecov bot commented Nov 18, 2021 •

edited

Loading

mx-psi Nov 19, 2021 •

edited

Loading

mx-psi Nov 19, 2021 •

edited

Loading

bogdandrutu Jan 18, 2022 •

edited

Loading