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

fix(config): Replace environment variables if existing but empty #13570

Merged
merged 2 commits into from
Jul 10, 2023

Conversation

srebhan
Copy link
Contributor

@srebhan srebhan commented Jul 7, 2023

resolves #13565

Previous attempts (e.g. PR #13457) to restore the pre-1.27.0 environment-variable-replacement behavior missed one corner case where the environment variable exists but is empty. Versions before v1.27.0 will replace those variables (with an empty string) but the current code does not (see #13565).

This PR restores the old behavior by using the additional information about the application of the default replacement, indicating if an interpolation happened or if the variable could be mapped.

@orgads
Copy link

orgads commented Jul 7, 2023

Is there a reason not to replace also non-existing vars with an empty string?

@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Jul 7, 2023

@orgads
Copy link

orgads commented Jul 7, 2023

I can't think of a use-case that would expect a value to be "foo" if FOO=foo is set, and "${FOO}" if it is unset. Is this intentional?

@srebhan
Copy link
Contributor Author

srebhan commented Jul 7, 2023

@orgads please think about e.g. #13460. The issue is that the pattern $... or ${...} is also used within config options that e.g. use go-templates or regular expressions. In any case, we want to keep the old (pre-1.27.0) behavior and this only replaces if the envvar exists.

@srebhan srebhan added regression something that used to work, but is now broken area/configuration ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. labels Jul 7, 2023
@srebhan srebhan removed the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Jul 7, 2023
@srebhan
Copy link
Contributor Author

srebhan commented Jul 7, 2023

Depends on compose-spec/compose-go#431.

@powersj powersj assigned srebhan and unassigned powersj Jul 7, 2023
Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to land once unit tests pass

@srebhan srebhan merged commit 1dd45b1 into influxdata:master Jul 10, 2023
21 of 22 checks passed
@srebhan srebhan deleted the config_issue_13565 branch July 10, 2023 13:24
@srebhan srebhan added this to the v1.27.2 milestone Jul 10, 2023
powersj pushed a commit that referenced this pull request Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/configuration fix pr to fix corresponding bug regression something that used to work, but is now broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[REG 2.6->2.7] Env variable expansion is not done when variable is empty
3 participants