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

feat(config): Support shell like syntax for environment variable substitution #13229

Merged
merged 10 commits into from
May 24, 2023

Conversation

neelayu
Copy link
Contributor

@neelayu neelayu commented May 3, 2023

Required for all PRs

resolves #10557

Support for shell like environment variable expansion. This PR uses a library used by docker-compose to expand variables from the environment. It was a conscious choice to use a library and not build on existing regex patterns. The library gives more flexibility and is actively maintained.

It builds on #10726 and removes the comments before it is passed on to the toml parser. Most cases are handled in the code. Let me know if I missed any. The reason to remove comments before env substitution was due to the fact the default config generated includes the text

# Environment variables can be used anywhere in this config file, simply surround
# them with ${}. For strings the variable must be within quotes (ie, "${STR_VAR}"),
# for numbers and booleans they should be plain (ie, ${INT_VAR}, ${BOOL_VAR})

More specifically the syntax ${} throws an error. So for backwards compatibility, the choice was made to remove the comments. The code to remove comments handles cases where # can be part of string values.

Kindly review. Thanks

@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label May 3, 2023
@neelayu neelayu changed the title feat(config): Support shell like sytax for environment variable substitution feat(config): Support shell like syntax for environment variable substitution May 3, 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.

Hi,

Thanks for putting this up, this will be a nice addition. We will obviously require a bit of testing to ensure we really do not regress anyone's config as they are today.

I would like the --env-file removed from this PR. That should be a second PR where we can discuss the reasoning and justification for another config file.

I have some initial comments and observations.

Thanks!

docs/CONFIGURATION.md Outdated Show resolved Hide resolved
docs/CONFIGURATION.md Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
config/testdata/inline_table.toml Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
@powersj powersj self-assigned this May 3, 2023
@neelayu neelayu force-pushed the env-subst branch 2 times, most recently from 08fe5fc to e6f05c4 Compare May 4, 2023 08:36
@neelayu neelayu force-pushed the env-subst branch 2 times, most recently from 228675a to 212c59f Compare May 5, 2023 12:26
@powersj
Copy link
Contributor

powersj commented May 19, 2023

I'm catching up on stuff post-vacation. I will take a look at this early next week after we do v1.26.3. I want to play with it a bit more.

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.

Love it! When using env variables with bool and numeric you get TOML validation errors, which is unfortunate, but understandable and that happens today anyway as well.

Went through and played with boolean for debug value and for hostname via string. Tried a few places to toss variables in as comments with no negative effect.

@powersj powersj added 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 May 23, 2023
@powersj powersj assigned srebhan and unassigned powersj May 23, 2023
@telegraf-tiger
Copy link
Contributor

@neelayu
Copy link
Contributor Author

neelayu commented May 24, 2023

Thanks for reviewing.
Can you share your config containing bool and numeric that you were testing? Just want to make sure, I didn't miss any case!

Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Nice. Thanks for the cool feature @neelayu!

@srebhan srebhan merged commit 7c636b4 into influxdata:master May 24, 2023
@powersj
Copy link
Contributor

powersj commented May 24, 2023

Can you share your config containing bool and numeric that you were testing? Just want to make sure, I didn't miss any case!

First, I should clarify. Telegraf does not complain about any validation errors, but my IDE will complain about invalid toml when you say debug = ${TELEGRAF_DEBUG}. I was setting the agent-level debug and metric_batch_size values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/agent area/configuration feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Environment variables: Allow default values or error messages if empty
4 participants