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

Add support for multiple config arguments #1576

Merged
merged 10 commits into from
Jun 6, 2022

Conversation

crobert-1
Copy link
Contributor

The core distribution of the collector now supports multiple "--config" input arguments, so this change updates the Splunk
distro to also support this. This change also adds new flag parsing functionality to use golang's flag library, instead of manual parsing. Testing has also been added to ensure the config server works for multiple files, flag parsing works as expected, and the collector can properly use multiple input config files.

@crobert-1 crobert-1 requested review from a team as code owners May 18, 2022 23:36
@dmitryax
Copy link
Contributor

@crobert-1 can you please take a look at the failing integration test TestSpecifiedContainerConfigDefaultsToCmdLineArgIfEnvVarConflict? I seems like it can be related

@crobert-1 crobert-1 marked this pull request as draft May 19, 2022 17:41
cmd/otelcol/flags.go Outdated Show resolved Hide resolved
@crobert-1 crobert-1 marked this pull request as ready for review May 19, 2022 20:04
cmd/otelcol/flags.go Outdated Show resolved Hide resolved
cmd/otelcol/flags.go Outdated Show resolved Hide resolved
cmd/otelcol/main.go Outdated Show resolved Hide resolved
cmd/otelcol/main.go Outdated Show resolved Hide resolved
@crobert-1 crobert-1 requested a review from a team as a code owner May 20, 2022 22:03
@crobert-1 crobert-1 force-pushed the support_multiple_configs branch 4 times, most recently from f56a20c to aa48708 Compare May 21, 2022 00:23
@crobert-1
Copy link
Contributor Author

crobert-1 commented May 21, 2022

(I'm investigating the failing test to see how it's related to these changes)
Failure is due to issue reference in PR #1595

@rmfitzpatrick
Copy link
Contributor

I'd strongly recommend adding an integration test that uses multiple configs to be able to better vet this over time. Maybe one file for a host metrics receiver, one for a processor, and another for an exporter?

go.mod Outdated Show resolved Hide resolved
@crobert-1
Copy link
Contributor Author

I'd strongly recommend adding an integration test that uses multiple configs to be able to better vet this over time. Maybe one file for a host metrics receiver, one for a processor, and another for an exporter?

Added an integration test to cover this case, thanks!

@crobert-1 crobert-1 force-pushed the support_multiple_configs branch 2 times, most recently from b97b5f3 to 1150486 Compare May 26, 2022 17:37
The core distribution of the collector now supports multiple
"--config" input arguments, so this change updates the Splunk
distro to also support this. Testing has also been added to
ensure the config server works for multiple files, flag parsing
works as expected, and the collector can properly use multiple
input config files.
Moving the flags into a single encapsulated struct helps organization
of variables and testability of the code.
- Encapsulate flags in single structure
- Remove getter methods that don't have any extra value
- Handle invalid flags passed in
- Mark variable as const
- Update CHANGELOG
- Comment why CLI flag usage strings are empty
- Remove go import replace statement
- Assert error contents in tests
-Add locking to config server so it doesn't need to be shutdown and
restarted, but can just be started and use references to serve
configs.
- Add integration test to make sure collector can be run with multiple
config files
Copy link
Contributor

@rmfitzpatrick rmfitzpatrick left a comment

Choose a reason for hiding this comment

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

Thanks!

@crobert-1 crobert-1 merged commit e62d378 into signalfx:main Jun 6, 2022
@crobert-1 crobert-1 deleted the support_multiple_configs branch June 6, 2022 21:37
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.

4 participants