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

support .env file to load env #2675

Closed
wants to merge 1 commit into from
Closed

support .env file to load env #2675

wants to merge 1 commit into from

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Mar 9, 2023

Fixes #2643

This adds an autoload of any .env file located in the working directory when invoking the collector.

A .env file may contain environment variables that then get loaded as part of the env vars seen by the collector.

This is a lightweight approach to fixing the issue.

@atoulme atoulme requested review from a team as code owners March 9, 2023 05:12
@rmfitzpatrick
Copy link
Contributor

rmfitzpatrick commented Mar 9, 2023

Can you update the advanced configuration section of the readme and add a unit or integration test* for the functionality?

@@ -22,6 +22,7 @@ import (
"log"
"os"

_ "github.com/joho/godotenv/autoload"
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the behavior of invalid .env content and would it be better to use the library directly to marshal the error to a proper handler, like in main or internal/settings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What behavior would you like to see if the content is invalid? Should the collector forcefully exit then, or is an error log ok?

Copy link
Contributor

@rmfitzpatrick rmfitzpatrick Mar 13, 2023

Choose a reason for hiding this comment

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

imo I'd expect there to be a logged warning about the validity of the file w/ the parsing error and for none of the env vars in the file to be sourced but can see other options being desirable. I'm not sure how feasible this is given we aren't controlling the implementation as is*. Terminating the collector seems overkill.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I'll factor that in.

@hughesjj-splunk
Copy link
Contributor

Can you update the advanced configuration section of the readme and add a unit or integration test* for the functionality?

+1 to this, and specifically it'd be nice to have an invocation example in the README

@atoulme
Copy link
Contributor Author

atoulme commented Mar 13, 2023

Upstream is talking about going with different binaries as an option: open-telemetry/opentelemetry-collector#7350

@atoulme atoulme marked this pull request as draft March 13, 2023 16:25
@atoulme atoulme closed this Mar 24, 2023
@atoulme atoulme deleted the support_env_file branch November 8, 2023 21:40
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.

Feature Request: command line option for NO_WINDOWS_SERVICE env var
4 participants