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

[collector] Use ${env:ENV} style syntax for env variable expansion #2136

Merged
merged 2 commits into from
Jan 3, 2023

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Dec 29, 2022

Uses new ${env:ENV} style syntax for environment variable expansion. This is the recommended way as of recent Collector versions.

Relates to open-telemetry/opentelemetry-collector-contrib/issues/17299

@mx-psi mx-psi requested review from a team as code owners December 29, 2022 12:52
@mx-psi mx-psi requested review from bogdandrutu and removed request for a team December 29, 2022 12:52
Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Thanks!

Do you think we should document this more thoroughly, like showing another example with a key different from env? I don't have a real sense for this but I wonder if some folks will get confused by the syntax since they may not be familiar with pwsh.

@svrnm
Copy link
Member

svrnm commented Dec 29, 2022

Thanks!

Do you think we should document this more thoroughly, like showing another example with a key different from env? I don't have a real sense for this but I wonder if some folks will get confused by the syntax since they may not be familiar with pwsh.

As someone who is not familiar with pwsh I am confused, so yes, let's have another example or link to some external docs that describe that syntax

@mx-psi
Copy link
Member Author

mx-psi commented Dec 29, 2022

I think it makes sense to add other config providers examples (probably after rewording this section to be a bit more general), but would like to hear what @bogdandrutu thinks first

@Aneurysm9
Copy link
Member

Is this actually PowerShell syntax, or simply something that shares some cosmetic similarities in the same way that the previous examples shared some cosmetic similarities with Bourne Shell expansion syntax but were not? If it is not actually PowerShell syntax (including defaults, substitutions, slicing, etc), then we should not mention PowerShell.

@cartermp
Copy link
Contributor

Hmm, I think I would be fine with saying something like "similar to PowerShell", but in general the structure of the syntax should get documented with at least one other example than env:SOMETHING.

@mx-psi
Copy link
Member Author

mx-psi commented Dec 30, 2022

Is this actually PowerShell syntax, or simply something that shares some cosmetic similarities in the same way that the previous examples shared some cosmetic similarities with Bourne Shell expansion syntax but were not?

Definitely the latter (at least at this point); I assumed if the previous wording was okay then this should also be (since we didn't support defaults, substitution or slicing with the previous format either), but probably the amount of people familiar with PowerShell syntax is much smaller and this is more confusing than helpful.

Does something like "The use and expansion of environment variables is supported in the Collector configuration. For example to use the values stored on the DB_KEY and OPERATION environment variables you can write the following:" sound better to you?

Hmm, I think I would be fine with saying something like "similar to PowerShell", but in general the structure of the syntax should get documented with at least one other example than env:SOMETHING.

I think that needs a bigger refactor on the docs, I would rather get the syntax updated first on this example and then open a second PR to reword this in terms of providers. I am happy to work on that second PR myself.

@Aneurysm9
Copy link
Member

Does something like "The use and expansion of environment variables is supported in the Collector configuration. For example to use the values stored on the DB_KEY and OPERATION environment variables you can write the following:" sound better to you?

Yes, I could get behind that. I just don't want to inadvertently give the impression that the collector has capabilities it doesn't.

@mx-psi
Copy link
Member Author

mx-psi commented Jan 3, 2023

Reworded as mentioned on #2136 (comment)

@cartermp
Copy link
Contributor

cartermp commented Jan 3, 2023

I think this is good enough for now (it could continue to improve but we can handle that later)

@cartermp
Copy link
Contributor

cartermp commented Jan 3, 2023

Thanks!

@cartermp cartermp merged commit 6d3eb45 into main Jan 3, 2023
@cartermp cartermp deleted the mx-psi-patch-1 branch January 3, 2023 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants