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

Refactor main flags as settings.Settings #1952

Merged
merged 2 commits into from
Sep 9, 2022
Merged

Conversation

rmfitzpatrick
Copy link
Contributor

@rmfitzpatrick rmfitzpatrick commented Sep 7, 2022

These changes move existing main and flags option parsing, env var evaluation/setting, and default logic to a new internal settings package where a more minimal Settings interface is used to populate the underlying config provider resolver settings and command line argument adjustment. They are largely attempted to be noops/transparent readability and logic changes with some test practices improvements except for:

  1. changing the std flag library to spf13/pflag so we can use the VarP helpers for version and help shorthands and not have to track with all collector flag names (something we're most likely failing to do already).
  2. Removing the feature flag value implementation and parsing since we don't use them or the feature registry at this time (which is ok now w/ the above).
  3. Remove the project-local default gateway and otlp linux config paths since they are only used by tests and none of the installers apply a cmd/-rooted location

I didn't change the bulk of the config and memory ballast logic to keep the change set more manageable and the likelihood of introducing bugs down, but think it could benefit from an overhaul in subsequent changes.*

@rmfitzpatrick rmfitzpatrick requested review from a team as code owners September 7, 2022 20:20
Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

The refactoring makes sense to me and makes main.go a lot easier to read. LGTM!

Copy link
Contributor

@pmcollins pmcollins left a comment

Choose a reason for hiding this comment

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

Big ups for removing code from main and making things testable 👍
LGTM

func (f *flags) ResolverURIs() []string {
var configPaths []string
if configPaths = f.configPaths.value; len(configPaths) == 0 {
if configEnvVal := os.Getenv(ConfigEnvVarName); len(configEnvVal) != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Long term, might be nice to separate Getenv and Setenv from the settings code since reading/writing them is effectively I/O, and instead pass this information around in some kind of abstraction that could be swappable for testing. Removes the need to deal with global (env) variables in tests.

Copy link
Contributor Author

@rmfitzpatrick rmfitzpatrick Sep 8, 2022

Choose a reason for hiding this comment

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

A thought/goal I've had is to determine a clear setting precedence of cli arg > env var > default that allows easily registering a new setting, readme update, and adding (to) an associated test. This would require settings reading the env though. Ideally there wouldn't be any side effects from setting values unless the only way to advertise their existence downstream is through the env. The final env interface could be something like Env() map[string]string that main or an applicable component would be responsible for actually sourcing to the environment*.

@rmfitzpatrick rmfitzpatrick merged commit 8fc9514 into main Sep 9, 2022
@delete-merged-branch delete-merged-branch bot deleted the moveflagstosettings branch September 9, 2022 18:42
hughesjj pushed a commit that referenced this pull request Sep 22, 2022
* Refactor service flags as Settings

* Remove test config locations as valid defaults
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