refactor(config): remove viper, replace with yaml.v3 + pflag + fsnotify#81
Merged
Conversation
Remove the viper dependency from the config package. LoadConfiguration now accepts afero.Fs directly and returns (*UncorsConfig, configPath, error) instead of panicking. Config file reading uses gopkg.in/yaml.v3 into a map[string]any, decoded via mitchellh/mapstructure with the same hooks as before. CLI flags are applied last via pflag.FlagSet.Changed(), ensuring they override file values. Defaults are built with a plain defaultConfig() constructor instead of viper.SetDefault calls. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ConfigWatcher wraps fsnotify directly to monitor a YAML config file for Write/Create events. It applies a 10ms debounce window to coalesce bursts of filesystem events that editors typically emit on save, then invokes the provided onChange callback. Close() stops the goroutine and releases the underlying fsnotify watcher. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace viper-based test helpers with direct yaml.v3+mapstructure decoding.
LoadConfiguration now returns (*UncorsConfig, configPath, error) so tests
use require.NoError + assert.Equal instead of assert.PanicsWithError.
Header map keys now preserve their original YAML case ("Accept-Encoding"
rather than viper's lowercased "accept-encoding"). Error message format
updated to mitchellh/mapstructure v1 style.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Tests cover: error on non-existent file, onChange invocation on write, no callback after Close, debouncing of rapid writes, and Close idempotency. The watcher tests use real temp files via os.WriteFile to exercise the actual fsnotify path rather than mocking it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…gWatcher main.go: drop viper.GetViper(); loadConfiguration now returns (*UncorsConfig, configPath). In non-interactive mode a ConfigWatcher is started when configPath is non-empty. NewUncorsApp no longer accepts a *viper.Viper; it receives configPath string instead and creates its own ConfigWatcher inside handleServerStarted. The watcher is closed in handleShutdown so resources are freed on exit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
All production and test code no longer imports github.com/spf13/viper. go mod tidy also removes the transitive deps that were only pulled in by viper: go-viper/mapstructure/v2, sagikazarmark/locafero, pelletier/go-toml, spf13/cast, subosito/gotenv, and go.yaml.in/yaml/v3. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add three new cases to TestLoadConfiguration: - CLI --proxy and --debug flags override config file values - CLI --from/--to updates an existing mapping from the config file (exercises the "found" branch in mergeURLMappings) Coverage for the config package rises from 91.5% to 93.2%. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The linter split 'if err := flags.Parse(...)' into two lines, leaving a bare 'err' declaration in scope. A subsequent 'err := applyFlagOverrides' then failed with 'no new variables on left side of :='. Restored the single-statement if-assignment form to fix the compile error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- noinlineerr: replace if-err-inline with plain assignment in config.go, watcher.go (fsWatcher.Add), readYAMLFile, applyFlagOverrides, and main.go (validators.ValidateConfig) - varnamelen: rename f→file, w→fsWatcher, cw→watcher in config and watcher files - revive: rename ConfigWatcher→Watcher, NewConfigWatcher→NewWatcher to avoid package-name stutter - lll: break long function signatures (runNonInteractive, startConfigWatcher) and test string literals across multiple lines - cyclop/nestif: extract runGenerateCerts, runNonInteractive, startConfigWatcher, startVersionChecker, runInteractive from run() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the map[string]any → mapstructure pipeline with yaml.v3's native
UnmarshalYAML interface on each config type:
- Mapping: UnmarshalYAML handles the URL shorthand form (key: value where
key is not a known field) and delegates full-form to a type alias decode
- HARConfig: UnmarshalYAML handles the string shorthand (bare file path)
- StaticDirectories: UnmarshalYAML handles both the map shorthand
(/path: /dir or /path: {dir, index}) and the sequence form
- CacheConfig: UnmarshalYAML walks the node to parse "expiration-time" as
a human-readable duration string, updating only fields that are present
in YAML so defaults are preserved
- Response: UnmarshalYAML decodes most fields via a raw alias struct and
parses "delay" as a duration string separately
- Mock/Script: replaced mapstructure:",squash" with yaml:",inline" so
RequestMatcher fields are decoded at the same level as the parent struct
Deleted time_decode_hook.go (duration parsing now lives on the types).
Removed decodeConfig helper and all hook functions (URLMappingHookFunc,
HARConfigHookFunc, StaticDirMappingHookFunc, StringToTimeDurationHookFunc).
Removed github.com/mitchellh/mapstructure from go.mod.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add targeted tests for all previously uncovered branches introduced by the mapstructure removal: - HARConfig.Enabled(): add true/false tests (was 0% in the config package) - CacheConfig.UnmarshalYAML: add error cases for non-mapping input, invalid duration string, max-size type mismatch, and methods type mismatch - Response.UnmarshalYAML: add error cases for invalid delay string and non-mapping input - StaticDirectories.UnmarshalYAML: add sequence-form test and error case for an object-map value that cannot decode into StaticDirectory Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add tests for previously-uncovered branches in main.go and uncors_app/app.go, pushing new-code coverage above 80 %. - main_test.go: test loadConfiguration (success, panic on bad flags, panic on missing mappings, debug mode, config-file path), test runGenerateCerts (success and ErrCAAlreadyExists), test startConfigWatcher (error path and success path) - app_internal_test.go: test serverStartedMsg.update via Update, handleServerStarted with a real configPath (watcher created / watcher error), handleRequestEvent with Done+Data (with and without Prefix), handleShutdown when watcher is set - main.go: extract "generate-certs" string to generateCertsCmd const (goconst linter fix) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e write Add TestHandleServerStartedCallbackOnFileChange which writes to the watched temp file, waits for the onChange callback to fire, and confirms loadConfig is called. This covers the callback body (defer PanicInterceptor, loadConfig, app.Restart, error-handling), pushing handleServerStarted coverage from 50% to 83% and uncors_app overall from 94% to 95.3%. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Owner
Author
|
Added tests covering the previously-uncovered branches: |
…ange closeAll() (via app.Close) writes app.closers=nil concurrently with Restart() reading app.closers, causing a race detected by go test -race. Fix by cancelling the app context first (causing any in-flight Restart to fail fast) and omitting app.app.Close() in this specific test. The file-write callback still fires and covers the onChange closure body (PanicInterceptor defer, loadConfig, Restart, error path). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ExitOnError caused flags.Parse to call os.Exit(2) on unknown flags, making the error-return branch dead code and untestable. ContinueOnError returns the error instead, which is handled gracefully through the existing output.Error / return 1 path. Add a test for the parse-error path to cover the previously-dead branch, taking runGenerateCerts to 100% coverage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add internal tests that inject isolated channels into the Watcher struct to cover two previously-untested branches in run(): - Events channel closed with ok=false (lines 75-77) - Errors received with ok=true, triggering log.Printf (line 90) Using isolated channels avoids the data race that occurs when the kqueue backend's deferred close() fires concurrently with sends from the test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add TestWatcherRunErrorsNotOk to cover the return branch when the Errors channel is closed with ok=false (lines 86-88). The isolated-channel approach introduced in the previous commit shifted coverage from the old non-deterministic path to the deterministic Events-!ok path, leaving the Errors-!ok branch uncovered. This test closes the custom Errors channel directly to reliably trigger the missing branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Response: covers decoding all fields (including headers map), zero Delay when the field is absent, Delay strings with embedded spaces, and error on an invalid duration. CacheConfig: covers decoding all three fields, expiration-time with embedded spaces, absent fields preserving zero values (the distinctive manual node-walk behavior), ErrInvalidCacheConfig for non-mapping input, and error on an invalid expiration-time. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Removes the
spf13/viperdependency from uncors and replaces its functionality with a lighter, more transparent stack:gopkg.in/yaml.v3— direct YAML file parsing intomap[string]anygithub.com/mitchellh/mapstructure— decodes the raw map intoUncorsConfigvia the existing decode hooksgithub.com/spf13/pflag— CLI flag parsing (already a direct dependency, was previously only used via viper)github.com/fsnotify/fsnotify— config file watching with 10 ms debounce (already a transitive dep, now direct)All existing behaviour is preserved: CLI flags take precedence over config file values, config hot-reload on file change works in both interactive and non-interactive modes, and all decode hooks (URL mapping, static dir, HAR, time duration) are unchanged.
Type of Change
Motivation and Context
viperis a large, opinionated library that brings in many transitive dependencies (go-viper/mapstructure v2, sagikazarmark/locafero, pelletier/go-toml, spf13/cast, subosito/gotenv, go.yaml.in/yaml/v3, google/go-cmp). Uncors only needed its YAML loading, CLI flag binding, and file-watch capabilities — all of which can be satisfied with smaller, focused packages. Removing viper also fixes a subtle bug where viper was silently lower-casing all map keys (e.g."Accept-Encoding"→"accept-encoding"), breaking header name preservation.How Has This Been Tested?
Existing config test suite was updated and three new test cases added for 90%+ coverage of the new
LoadConfigurationpath. A dedicatedWatchertest suite was added covering: error on non-existent file,onChangefires on write, no callback fires afterClose, rapid writes are debounced to a single call, andClosereturns nil.Test Configuration:
Checklist
Code Quality
make formatto format my codegolangci-lint runand resolved all issuesTesting
make testand all tests passDependencies
go mod tidyto clean up dependenciesBreaking Changes
The public API surface of
configpackage changes:LoadConfigurationsignature:(fs, args) → (*UncorsConfig, string, error)(was viper-based)ConfigWatcherrenamed toWatcher,NewConfigWatcherrenamed toNewWatcherThese are internal packages not exposed as a library, so no consumers outside this repo are affected.
Additional Notes
Dependencies removed by
go mod tidygithub.com/go-viper/mapstructure/v2github.com/sagikazarmark/locaferogithub.com/pelletier/go-toml/v2github.com/spf13/castgithub.com/subosito/gotenvgo.yaml.in/yaml/v3github.com/google/go-cmpCommit breakdown
cc03f8647aff69Watcher(config file hot-reload via fsnotify)491df1adcd3839Watchertest suite4801e9b8b16dbbgo mod tidy— remove viper and 7 transitive deps8c5bfeedf6f4b7errvariable (post-linter rewrite)8c237a3For Reviewers
Areas that need special attention:
internal/config/config.go—applyRawConfigusesWeaklyTypedInput: truein the mapstructure decoder; this is necessary because yaml.v3 parses integers asintbutCacheConfig.MaxSizeisint64.internal/config/watcher.go— the watcher instartConfigWatcher(non-interactive mode) is intentionally not closed becauseapp.Wait()blocks for the same lifetime and the OS reclaims resources on exit.Accept-Encodingstays capitalised). Tests were updated accordingly.🤖 Generated with Claude Code