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

feat: [V2] Rewrite config code without Viper #654

Merged
merged 19 commits into from
May 8, 2023
Merged

Conversation

kentquirk
Copy link
Contributor

@kentquirk kentquirk commented Apr 2, 2023

This is bigger than I wanted it to be, but there were a lot of tests that needed to be updated. I did rework a number of tests to simplify the creation of sample configurations, and eliminating Viper required the creation of a couple of new functions.

I feel like this has significantly simplified the config code and made it easier to understand and maintain. Please read the comments below before reviewing the code.

Elements of this project:

  • Separate cmdline/env from configuration
  • Find/write appropriate libraries to do all the tasks of loading cmdline, applying defaults, applying the cmdline, and validating afterward
  • Replace viper with these new libraries within the same config structure
  • Do the automatic reload (I wasn't going to do it for this PR but it breaks lots of tests, so I'm adding it).

Out of scope for this PR:

  • Change config internal structure and helper functions
  • Change sample configs and test code to YAML

How to tell if we're done?

  • Passes all its tests
  • Existing config files can still be loaded

Which problem is this PR solving?

Viper is a strongly opinionated configuration tool. That can make it easy to start building something, but because in this case "opinionated" means "there's one way to do it and no easy way to change that" it can (and has, in my experience) become more and more of a maintenance problem as a codebase ages.

Refinery needs a significant rework to its configuration model:

  • Its many config values should be broken up into more sections of related items and fewer at the root level
  • This will allow better unit testing without mocking the entire config in a single object
  • The relationship between command line, environment variables, and configuration should be clear and unambiguous
  • Default values should be clear and unambiguous
  • Since some configuration is reloadable at runtime, how that happens should be clear
  • We want to support loading config from a remote source
  • We want YAML-first configuration so that it's easier to work in a Kubernetes world
  • We want to enable config to be loaded from a URL
  • Full validation of configuration should become possible, including extra or mistyped names (this PR will not include this but will enable it).

Some of these things are possible with viper, but not all of them, and in practice Viper makes balancing these things difficult.

Short description of the changes

This PR introduces some new tools to enable us to remove Viper:

  • Configuration defaults are applied by a new library (creasty/defaults) that simply examines default struct tags and applies them when called.
  • A new library (jessevdk/go-flags) is used to handle command line and environment variables and map them onto a struct. This enables clear control of a separate struct (CmdEnv) for these things.
  • Items in the configuration that are tied to the CmdEnv struct are given the cmdenv struct tag in the configuration that identifies them.
  • A reflection-based function applies these cmdenv tags to configuration values at the appropriate time (after the configuration is loaded and defaults are applied).
  • We continue to use the existing validation library (go-playgroundvalidator) to express simple post-loading validations
  • These tools also allow loading a config from a URL instead of a file
  • There's a new config value ConfigReloadInterval that controls the timing of reloads; if the hash of the contents is the same, no reload occurs. This is a timed reload rather than one that depends on operating system notifications. A future PR will allow a cluster of refineries to share knowledge of a configuration change.

The result is that entries in configuration structs can have up to 3 primary tags:

  • default sets default values
  • cmdenv when they need to be configurable on the command line and in the environment
  • validate expresses validation rules

@kentquirk kentquirk added type: enhancement New feature or request wip Work In Progress breaking-change Prefer 'version: bump major', but use this for breaking changes that don't bump major. labels Apr 2, 2023
@kentquirk kentquirk self-assigned this Apr 2, 2023
@kentquirk kentquirk changed the title feat: [V2] WIP: configuration support code feat: [V2] Rewrite config code without Viper Apr 10, 2023
@kentquirk kentquirk marked this pull request as ready for review April 10, 2023 18:16
@kentquirk kentquirk requested a review from a team as a code owner April 10, 2023 18:16
cmd/refinery/main.go Show resolved Hide resolved
cmd/refinery/main.go Show resolved Hide resolved
config/file_config.go Show resolved Hide resolved
config/file_config.go Show resolved Hide resolved
config/file_config.go Outdated Show resolved Hide resolved
config/file_config.go Show resolved Hide resolved
config/file_config.go Show resolved Hide resolved
config/file_config.go Outdated Show resolved Hide resolved
config/file_config.go Outdated Show resolved Hide resolved
tools/convert/main.go Outdated Show resolved Hide resolved
@kentquirk
Copy link
Contributor Author

Tests broke; will fix.

@kentquirk kentquirk requested a review from TylerHelmuth April 13, 2023 21:24
@kentquirk kentquirk changed the base branch from main to kent.v2 May 8, 2023 15:01
@kentquirk kentquirk changed the base branch from kent.v2 to main May 8, 2023 19:09
@kentquirk kentquirk requested a review from TylerHelmuth May 8, 2023 19:39
@kentquirk kentquirk merged commit 9bb51b3 into main May 8, 2023
@kentquirk kentquirk deleted the kent.v2.new_config branch May 8, 2023 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Prefer 'version: bump major', but use this for breaking changes that don't bump major. type: enhancement New feature or request wip Work In Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants