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(config): use viper.Unmarshal capabilities #1079

Merged
merged 14 commits into from
Oct 20, 2022
Merged

Conversation

GeorgeMac
Copy link
Member

@GeorgeMac GeorgeMac commented Oct 18, 2022

This change is in reaction to a comment that went by in the last configuration refactor PR.

Currently, we do lots of viper.IsSet and viper.Get interrogation and extraction.
This PR changes the strategy to lean into viper.Unmarshal capabilities on the entire config.Config structure.
Along with a little sugar to aid in keeping the configuration files focussed.

To add a field to an existing configuration, you now simply amend the structure:

type CorsConfig struct {
	Enabled        bool     `json:"enabled" mapstructure:"enabled"`
	AllowedOrigins []string `json:"allowedOrigins,omitempty" mapstructure:"allowed_origins"`
+	AllowedMethods []string `json:"allowedMethods,omitempty" mapstructure:"allowed_methods"`
}

To set a default, you ensure the sub-configuration implements the defaulter interface.
In the setDefaults method you add your default like so:

func (c *CorsConfig) setDefaults(v *viper.Viper) []string {
	v.SetDefault("cors", map[string]any{
		"allowed_origins": "*",
+		"allowed_methods": "GET,PUT,POST",
	})

	return nil
}

To validate the resulting configuration is appropriate, the sub-configuration can implement the validator interface.

func (c *CorsConfig) validate() error {
	for _, method := range c.AllowedMethods {
		if _, ok := map[string]struct{}{
			"GET": struct{}{},
			"PUT": struct{}{},
			"POST": struct{}{},
		}[method]; !ok {
			return nil, fmt.Errorf("%q: %w", method, errValidationMethodNotAllowed)
		}
	}
	
	return nil
}

@GeorgeMac GeorgeMac requested a review from markphelps October 18, 2022 13:14
@GeorgeMac GeorgeMac changed the title refactor(config): user viper.Unmarshal capabilities refactor(config): use viper.Unmarshal capabilities Oct 18, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #1079 (f029ca3) into main (83469f2) will decrease coverage by 0.36%.
The diff coverage is 97.16%.

@@            Coverage Diff             @@
##             main    #1079      +/-   ##
==========================================
- Coverage   81.16%   80.80%   -0.37%     
==========================================
  Files          26       26              
  Lines        1954     1870      -84     
==========================================
- Hits         1586     1511      -75     
+ Misses        285      279       -6     
+ Partials       83       80       -3     
Impacted Files Coverage Δ
internal/config/config.go 76.71% <91.30%> (-10.33%) ⬇️
internal/config/cache.go 100.00% <100.00%> (ø)
internal/config/cors.go 100.00% <100.00%> (ø)
internal/config/database.go 100.00% <100.00%> (ø)
internal/config/log.go 100.00% <100.00%> (+16.66%) ⬆️
internal/config/meta.go 100.00% <100.00%> (+27.27%) ⬆️
internal/config/server.go 100.00% <100.00%> (ø)
internal/config/tracing.go 100.00% <100.00%> (+54.54%) ⬆️
internal/config/ui.go 100.00% <100.00%> (ø)
... and 4 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

Amazing work! I wonder in the future if we could/should separate out 'warnings' into it's own step/interface so that it doesn't fall on the responsibility of the defaults 'step'? Not necessary at the moment I don't think given we only have 2 warnings.

Thank you for doing this!!! 🥇

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.

3 participants