chore: v2 cleanup config for consistency by markphelps · Pull Request #4260 · flipt-io/flipt

markphelps · 2025-05-28T17:55:22Z

Cleaning up some inconsistencies in config default/validation that have been bugging me
Also I make more use of the IsZero method for YAML marshalling so we dont marshal default config for configs that are not enabled by default (ie: authn/authz/cors)

CoPilot Summary

This pull request introduces a series of improvements and refactorings to the configuration system, focusing on better validation, streamlined default setting, and enhanced error reporting. The most significant changes include replacing nested map configurations with flat key-value pairs for defaults, improving error messages with structured error wrapping, and introducing IsZero methods for various configuration structs to facilitate YAML marshaling.

Configuration Refactoring and Simplification:

Replaced nested map-based default configurations with flat v.SetDefault calls in authentication, authorization, analytics, cors, and diagnostics configurations for improved readability and maintainability. ([[1]](https://github.com/flipt-io/flipt/pull/4260/files#diff-9b81b9c76a5ad1bd617096d6af8c6bed965ca6c6b2505ce329d9d74151d0709cL84-L113), [[2]](https://github.com/flipt-io/flipt/pull/4260/files#diff-0a98ce1770fe73bf8c1e049ad80ee626b3b0d0bf0a6f8602cb5a8803aa0ce46eL25-R32), [[3]](https://github.com/flipt-io/flipt/pull/4260/files#diff-aa2f5b6adc90205848d8b017292d610c0116cdb91898380be186ab530c8b5fe6R56-R99), [[4]](https://github.com/flipt-io/flipt/pull/4260/files#diff-9610c88fc96f562246494f72b1eaeff59e104154ccc636f919a8020506923d0fL19-R36), [[5]](https://github.com/flipt-io/flipt/pull/4260/files#diff-c2ad2cf941769d35f9f8178105f661e7da3d4679ca1cf5caf55318e871b3f9c5L18-R18))
Removed the redundant BufferConfig struct and replaced it with AnalyticsBufferConfig for analytics buffering settings. ([[1]](https://github.com/flipt-io/flipt/pull/4260/files#diff-aa2f5b6adc90205848d8b017292d610c0116cdb91898380be186ab530c8b5fe6R56-R99), [[2]](https://github.com/flipt-io/flipt/pull/4260/files#diff-54c7c1af5fa8d5db4dc49f0e8e80e93ba2b1183ba4d5c9e2e5729e6deae6a3cdL531-L537))

Enhanced Error Reporting:

Replaced generic error messages with structured error wrapping using errFieldWrap and errFieldRequired for better context in error handling across multiple configurations, including authentication, authorization, credentials, and environments. ([[1]](https://github.com/flipt-io/flipt/pull/4260/files#diff-9b81b9c76a5ad1bd617096d6af8c6bed965ca6c6b2505ce329d9d74151d0709cL133-R123), [[2]](https://github.com/flipt-io/flipt/pull/4260/files#diff-0a98ce1770fe73bf8c1e049ad80ee626b3b0d0bf0a6f8602cb5a8803aa0ce46eL58-R45), [[3]](https://github.com/flipt-io/flipt/pull/4260/files#diff-550a5e46040e0aad0c3e8d167790482081155c813a487fc936af6eb3f8d23819L73-R91), [[4]](https://github.com/flipt-io/flipt/pull/4260/files#diff-655978c88529deb748e0b4ff7ee92445101433cda0956c7f79fc9fc623fc1489L44-R48))
Updated test cases to reflect the new error message structure, providing more detailed and consistent error descriptions. ([[1]](https://github.com/flipt-io/flipt/pull/4260/files#diff-78b78dc33eda1ed8d509bfa5a00f66709af57bd30d971cbaec454571a91c3e62L615-R630), [[2]](https://github.com/flipt-io/flipt/pull/4260/files#diff-78b78dc33eda1ed8d509bfa5a00f66709af57bd30d971cbaec454571a91c3e62L723-R733))

Validation Improvements:

Added stricter validation logic for configurations, such as ensuring required fields are present and validating mutually exclusive settings. ([[1]](https://github.com/flipt-io/flipt/pull/4260/files#diff-9b81b9c76a5ad1bd617096d6af8c6bed965ca6c6b2505ce329d9d74151d0709cL159-R148), [[2]](https://github.com/flipt-io/flipt/pull/4260/files#diff-655978c88529deb748e0b4ff7ee92445101433cda0956c7f79fc9fc623fc1489L65-R65), [[3]](https://github.com/flipt-io/flipt/pull/4260/files#diff-550a5e46040e0aad0c3e8d167790482081155c813a487fc936af6eb3f8d23819L126-R138))
Introduced IsZero methods for AnalyticsConfig, AuthorizationConfig, and CorsConfig to determine if configurations are enabled, aiding in YAML serialization. ([[1]](https://github.com/flipt-io/flipt/pull/4260/files#diff-aa2f5b6adc90205848d8b017292d610c0116cdb91898380be186ab530c8b5fe6R56-R99), [[2]](https://github.com/flipt-io/flipt/pull/4260/files#diff-0a98ce1770fe73bf8c1e049ad80ee626b3b0d0bf0a6f8602cb5a8803aa0ce46eL25-R32), [[3]](https://github.com/flipt-io/flipt/pull/4260/files#diff-9610c88fc96f562246494f72b1eaeff59e104154ccc636f919a8020506923d0fL19-R36))

Default Configuration Updates:

Updated the Default function to include more comprehensive default values for authorization and analytics configurations, ensuring consistency with the new flat-key approach. ([[1]](https://github.com/flipt-io/flipt/pull/4260/files#diff-54c7c1af5fa8d5db4dc49f0e8e80e93ba2b1183ba4d5c9e2e5729e6deae6a3cdL614), [[2]](https://github.com/flipt-io/flipt/pull/4260/files#diff-54c7c1af5fa8d5db4dc49f0e8e80e93ba2b1183ba4d5c9e2e5729e6deae6a3cdL631-R635))

Other Changes:

Simplified and clarified error handling in credentials and environments configurations by using structured error messages and validations. ([[1]](https://github.com/flipt-io/flipt/pull/4260/files#diff-550a5e46040e0aad0c3e8d167790482081155c813a487fc936af6eb3f8d23819L20-R19), [[2]](https://github.com/flipt-io/flipt/pull/4260/files#diff-655978c88529deb748e0b4ff7ee92445101433cda0956c7f79fc9fc623fc1489L24-R24))

Signed-off-by: Mark Phelps <[email protected]>

Copilot

Pull Request Overview

This pull request cleans up the configuration system for consistency by replacing nested maps with flat key–value defaults, enhancing error reporting through structured error wrapping, and introducing IsZero methods for YAML marshalling. Key changes include refactoring default-setting calls across multiple configuration modules, updating error messages for better clarity, and simplifying test expectations to reflect the new error structure.

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
internal/config/ui.go	Replaced map-based defaults with individual key defaults
internal/config/tracing.go	Updated default for tracing.enabled
internal/config/testdata/marshal/yaml/default.yml	Removed nested configuration blocks for analytics and cors
internal/config/testdata/analytics/invalid_buffer_configuration_flush_period.yml	Added storage configuration in analytics tests
internal/config/storage.go	Changed error wrapping for storage validations
internal/config/server.go	Changed default settings from a map to flat key defaults
internal/config/metrics.go, meta.go, log.go	Converted map defaults to individual calls
internal/config/environments.go	Updated error messages using errFieldWrap and errFieldRequired
internal/config/diagnostics.go	Simplified defaults for diagnostics profiling
internal/config/credentials.go	Updated error messages for credential validations
internal/config/cors.go	Converted default settings to flat keys and added an IsZero method
internal/config/config_test.go	Adjusted expected error messages in tests
internal/config/config.go	Removed BufferConfig in favor of AnalyticsBufferConfig alongside updated Authorization defaults
internal/config/authorization.go	Refactored setDefaults to use flat keys and added an IsZero method
internal/config/authentication.go	Refactored authentication defaults and validations; updated error messages
internal/config/analytics.go	Changed default settings to flat key defaults and adjusted validations

Copilot · 2025-05-28T17:56:11Z

+func (c *AuthenticationSessionStorageConfig) setDefaults(v *viper.Viper) error {
+	v.SetDefault("type", AuthenticationSessionStorageTypeMemory)
+
+	return c.Redis.setDefaults(v)


The new setDefaults implementation for AuthenticationSessionStorageConfig always calls c.Redis.setDefaults(v) without checking c.Type. This may lead to a nil dereference when the storage type is not Redis. Consider adding a conditional check to call c.Redis.setDefaults(v) only if c.Type equals AuthenticationSessionStorageTypeRedis, and return nil otherwise.

Suggested change

return c.Redis.setDefaults(v)

if c.Type == AuthenticationSessionStorageTypeRedis {

return c.Redis.setDefaults(v)

}

return nil

codecov · 2025-05-28T18:03:00Z

Codecov Report

Attention: Patch coverage is 77.06422% with 25 lines in your changes missing coverage. Please review.

Please upload report for BASE (v2@7d464f7). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
internal/config/authentication.go	61.53%	9 Missing and 1 partial ⚠️
internal/config/credentials.go	58.33%	5 Missing ⚠️
internal/config/storage.go	0.00%	4 Missing ⚠️
internal/config/authorization.go	66.66%	3 Missing ⚠️
internal/config/environments.go	40.00%	3 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##             v2    #4260   +/-   ##
=====================================
  Coverage      ?   50.75%           
=====================================
  Files         ?      121           
  Lines         ?    12707           
  Branches      ?        0           
=====================================
  Hits          ?     6450           
  Misses        ?     5728           
  Partials      ?      529

Flag	Coverage Δ
unittests	`50.75% <77.06%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

erka

nice!

erka · 2025-05-28T18:23:12Z

+func (c *AuthorizationConfig) setDefaults(v *viper.Viper) error {
+	v.SetDefault("authorization.required", false)
+	v.SetDefault("authorization.backend", AuthorizationBackendLocal)
+	v.SetDefault("authorization.local.policy.poll_interval", 5*time.Minute)


Suggested change

v.SetDefault("authorization.local.policy.poll_interval", 5*time.Minute)

v.SetDefault("authorization.local.policy.poll_interval", 5*time.Minute)

v.SetDefault("authorization.local.data.poll_interval", 30*time.Second)

Signed-off-by: Mark Phelps <[email protected]>

* v2: chore: v2 cleanup config for consistency (#4260) fix(ui): v2 improve flag rules controls (#4259) chore(deps): bump some vulnerable deps (#4258) build: v2 remove unused dependencies in Dockerfile (#4257)

chore: cleanup config for consistency

1924f58

Signed-off-by: Mark Phelps <[email protected]>

markphelps requested a review from a team as a code owner May 28, 2025 17:55

dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label May 28, 2025

markphelps requested review from Copilot and erka May 28, 2025 17:55

Copilot AI reviewed May 28, 2025

View reviewed changes

erka approved these changes May 28, 2025

View reviewed changes

chore: set data poll interval if configured

90e773a

Signed-off-by: Mark Phelps <[email protected]>

markphelps merged commit a4214cd into v2 May 28, 2025

markphelps deleted the v2-config-cleanup branch May 28, 2025 18:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

chore: v2 cleanup config for consistency#4260

chore: v2 cleanup config for consistency#4260
markphelps merged 2 commits into
v2from
v2-config-cleanup

markphelps commented May 28, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI May 28, 2025

Uh oh!

codecov Bot commented May 28, 2025

Uh oh!

erka left a comment

Uh oh!

erka May 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	v.SetDefault("authorization.local.policy.poll_interval", 5*time.Minute)
	v.SetDefault("authorization.local.policy.poll_interval", 5*time.Minute)
	v.SetDefault("authorization.local.data.poll_interval", 30*time.Second)

Uh oh!

Uh oh!

Conversation

markphelps commented May 28, 2025

CoPilot Summary

Configuration Refactoring and Simplification:

Enhanced Error Reporting:

Validation Improvements:

Default Configuration Updates:

Other Changes:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 28, 2025

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented May 28, 2025

Codecov Report

Uh oh!

erka left a comment

Choose a reason for hiding this comment

Uh oh!

erka May 28, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants