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

[confmap] Start deprecation processor for env var expand syntax ($ENV) #10144

Conversation

TylerHelmuth
Copy link
Member

Description

Starts the deprecation for support of env var expand syntax ($ENV) by adding a feature gate and a deprecation warning

Link to tracking issue

Related to #8215

Testing

Local testing

@TylerHelmuth TylerHelmuth requested review from atoulme and mx-psi May 13, 2024 22:21
// PreventEnvVarExpansionFeatureGate is the feature gate that controls whether the collector
// supports configuring the OpenTelemetry SDK via configuration
var PreventEnvVarExpansionFeatureGate = featuregate.GlobalRegistry().MustRegister(
"confmap.preventEnvVarExpansion",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"confmap.preventEnvVarExpansion",
"confmap.NoBracelessEnvVars",

I think we should refine the name a bit, since we will still allow env var expansion even after the deprecation process. We could also say NoNakedEnvVars, but I think "braceless" is a bit clearer to someone who isn't familiar with our terminology.

Copy link
Member

Choose a reason for hiding this comment

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

Should we have a single feature gate for all changes related to the environment variable revamp? Or should we have one for each change? I am inclined to say we should have one (easier to figure out how to revert back to the old behavior without having to understand its intricacies)

Copy link
Member Author

Choose a reason for hiding this comment

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

What is another change? I thought $ENV was the only thing we were deprecating

Copy link
Member

Choose a reason for hiding this comment

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

Making types stricter and unified between ${ENV} and ${env:ENV} is the other part

Comment on lines +20 to +21
// PreventEnvVarExpansionFeatureGate is the feature gate that controls whether the collector
// supports configuring the OpenTelemetry SDK via configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// PreventEnvVarExpansionFeatureGate is the feature gate that controls whether the collector
// supports configuring the OpenTelemetry SDK via configuration
// PreventEnvVarExpansionFeatureGate disables the ability to use environment variables
// without wrapping the variable in braces.

I don't think we need to explicitly call out SDK configuration here; I also don't think this has to do with the Collector's SDK instance.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like I had a bad copy-paste

@TylerHelmuth
Copy link
Member Author

I opened this before seeing #7111, looks like we need to do some work to allow ${} without of expandconverter. Closing this for now, I'll work on #7111 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants