Skip to content

It seems to be impossible to use environment variable configuration in the builder #8239

Closed
@echlebek

Description

@echlebek

Describe the bug
Recently, I noticed that the builder has deprecation warnings for the --go, --output-path, and --version. The builder advises:

Flag --version has been deprecated, use config distribution::version

But, how can I set distribution::version? Well first of all, I need to know that distribution actually means "dist" in the yaml config file. OK, check. So I can move things into the config file, and that works as expected. However, I need to set some properties like distribution::go dynamically, and the config file is not great for doing that. So I look at the source of the builder and see that environment variable handling is set up. But setting an environment variable such as dist_go seems to have no effect.

Steps to reproduce
Set an environment variable of dist_go, dist_version, or dist_output_path before running the builder.

What did you expect to see?
I expect to see the default go, version, or output_path overridden by the environment variable.

What did you see instead?
The default is set instead.

What version did you use?
5b47503

What config did you use?
config

Environment
OS: Fedora 37
Compiler: go1.20 linux/amd64

Additional context
It seems that the callback for the environment variable parser is called backwards. See https://github.com/open-telemetry/opentelemetry-collector/blob/main/cmd/builder/internal/command.go#L137 vs the example https://github.com/knadh/koanf/blob/master/README.md?plain=1#L251.

It would also be nice if the env var parser converted uppercase config into lowercase.

Finally, I'd like to note that the builder defaults to a go from PATH if the configured go cannot successfully be run. While this might be helpful to some users, I found it confusing, because I deliberately configured a bad path to go while I was debugging. I think this could actually be a footgun for users that make a mistake in their configured path to go, in that instead of an error, they will get a silent replacement.

Edit: one last thing. It seems like the environment variable strategy has an inherent flaw as well, because underscores are meant to be treated as separators, and separators are treated as nesting cues by koanf. So a key like dist_output_path would be translated to {"dist": {"output": {"path": "foo"}}} instead of {"dist": {"output_path": "foo"}}.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions