From 6301a1c56c9abc87ed466a1955573da2db66d9b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Rainone?= Date: Wed, 16 Sep 2020 16:08:21 +0200 Subject: [PATCH 01/13] Move hasConfig in config.go --- config.go | 6 ++++++ help.go | 5 ----- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/config.go b/config.go index 489c7957..6c9fefc6 100644 --- a/config.go +++ b/config.go @@ -360,3 +360,9 @@ func NewConfigFromToml(f io.Reader, comp Components) (*Config, error) { // Fill-in with missing defaults return &cfg, cfg.fillDefaults() } + +// hasConfig returns true if the underlying structure has at least one field. +func hasConfig(cfg interface{}) bool { + tf := reflect.TypeOf(cfg).Elem() + return tf.NumField() != 0 +} diff --git a/help.go b/help.go index 056476d9..0215ba22 100644 --- a/help.go +++ b/help.go @@ -129,11 +129,6 @@ func PrintHelp(w io.Writer, name string, comp Components) { } } -func hasConfig(cfg interface{}) bool { - tf := reflect.TypeOf(cfg).Elem() - return tf.NumField() != 0 -} - func dumpConfigHelp(w io.Writer, cfg interface{}) { const sfmt = "%-18s | %-18s | %-26s | " const sep = "----------------------------------------------------------------------------------------------------" From 1cd5246003d9842859fabfcd0d81eadc23efcb62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Rainone?= Date: Wed, 16 Sep 2020 16:15:35 +0200 Subject: [PATCH 02/13] lint: don't use underscores in Go names --- config_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/config_test.go b/config_test.go index 63dd6dcd..19e81a13 100644 --- a/config_test.go +++ b/config_test.go @@ -67,14 +67,14 @@ func TestFillCreateRecordDefault(t *testing.T) { } func TestEnvVarBaseReplace(t *testing.T) { - src_toml := ` + src := ` [general] dont_validate_fields = ${DNT_VAL_FIELDS} alt_form = "$ALT_FORM" unexisting_var = "${THIS_DOESNT_EXIST}" ` - want_toml := ` + want := ` [general] dont_validate_fields = true alt_form = "ok" @@ -91,13 +91,13 @@ func TestEnvVarBaseReplace(t *testing.T) { return "" } - s, err := replaceEnvVars(strings.NewReader(src_toml), mapper) + s, err := replaceEnvVars(strings.NewReader(src), mapper) if err != nil { t.Fatalf("replaceEnvVars err: %v", err) } buf, _ := ioutil.ReadAll(s) - if want_toml != string(buf) { + if want != string(buf) { t.Fatalf("wrong toml: %s", string(buf)) } } From 3d5ca1fa521fd7592f614115b0365a9fb9c25961 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Rainone?= Date: Wed, 16 Sep 2020 16:22:02 +0200 Subject: [PATCH 03/13] Add RequiredFields function + test --- config.go | 23 +++++++++++++++++++++++ config_test.go | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/config.go b/config.go index 6c9fefc6..e8818c3c 100644 --- a/config.go +++ b/config.go @@ -366,3 +366,26 @@ func hasConfig(cfg interface{}) bool { tf := reflect.TypeOf(cfg).Elem() return tf.NumField() != 0 } + +// RequiredFields returns the names of the underlying configuration structure +// fields which are tagged as required. To tag a field as being required, a +// "required" struct struct tag must be present and set to true. +// +// RequiredFields doesn't support struct embedding other structs. +func RequiredFields(cfg interface{}) []string { + var fields []string + + tf := reflect.TypeOf(cfg).Elem() + for i := 0; i < tf.NumField(); i++ { + field := tf.Field(i) + + req := field.Tag.Get("required") + if req != "true" { + continue + } + + fields = append(fields, field.Name) + } + + return fields +} diff --git a/config_test.go b/config_test.go index 19e81a13..ec316fd9 100644 --- a/config_test.go +++ b/config_test.go @@ -2,6 +2,7 @@ package baker import ( "io/ioutil" + "reflect" "strings" "testing" ) @@ -101,3 +102,51 @@ func TestEnvVarBaseReplace(t *testing.T) { t.Fatalf("wrong toml: %s", string(buf)) } } + +func TestRequiredFields(t *testing.T) { + type ( + test1 struct { + Name string + Value string `help:"field value" required:"false"` + } + + test2 struct { + Name string + Value string `help:"field value" required:"true"` + } + + test3 struct { + Name string `required:"true"` + Value string `help:"field value" required:"true"` + } + ) + + tests := []struct { + name string + cfg interface{} + want []string + }{ + { + name: "no required fields", + cfg: &test1{}, + want: nil, + }, + { + name: "one required field", + cfg: &test2{}, + want: []string{"Value"}, + }, + { + name: "all required fields", + cfg: &test3{}, + want: []string{"Name", "Value"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := RequiredFields(tt.cfg); !reflect.DeepEqual(got, tt.want) { + t.Errorf("RequiredFields() = %v, want %v", got, tt.want) + } + }) + } +} From e6aa8411f052e1a3ac083835a799ad5d7eb97c61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Rainone?= Date: Wed, 16 Sep 2020 17:00:25 +0200 Subject: [PATCH 04/13] Add CheckRequiredFields + test --- config.go | 21 +++++++++++++++++ config_test.go | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+) diff --git a/config.go b/config.go index e8818c3c..4da7e1b8 100644 --- a/config.go +++ b/config.go @@ -389,3 +389,24 @@ func RequiredFields(cfg interface{}) []string { return fields } + +// CheckRequiredFields checks that all fields that are tagged as required in +// val's type have actually been set to a value other than the field type zero +// value. If not CheckRequiredFields returns the name of the first required +// field that is not set, or, it returns an empty string if all required fields +// are set of the struct doesn't have any required fields (or any fields at all). +// +// CheckRequiredFields doesn't support struct embedding other structs. +func CheckRequiredFields(val interface{}) string { + fields := RequiredFields(val) + + for _, name := range fields { + rv := reflect.ValueOf(val).Elem() + fv := rv.FieldByName(name) + if fv.IsZero() { + return name + } + } + + return "" +} diff --git a/config_test.go b/config_test.go index ec316fd9..79610e0d 100644 --- a/config_test.go +++ b/config_test.go @@ -150,3 +150,66 @@ func TestRequiredFields(t *testing.T) { }) } } + +func TestCheckRequiredFields(t *testing.T) { + type ( + test1 struct { + Name string + Value string `help:"field value" required:"false"` + } + + test2 struct { + Name string + Value string `help:"field value" required:"true"` + } + + test3 struct { + Name string `required:"true"` + Value string `help:"field value" required:"true"` + } + ) + + tests := []struct { + name string + val interface{} + want string + }{ + { + name: "no required fields", + val: &test1{}, + want: "", + }, + { + name: "one missing required field ", + val: &test2{Name: "name", Value: ""}, + want: "Value", + }, + { + name: "one present required field ", + val: &test2{Name: "name", Value: "value"}, + want: "", + }, + { + name: "all required fields and all are missing", + val: &test3{}, + want: "Name", + }, + { + name: "all required fields but the first missing", + val: &test3{Value: "value"}, + want: "Name", + }, + { + name: "all required fields and all are present", + val: &test3{Name: "name", Value: "value"}, + want: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := CheckRequiredFields(tt.val); got != tt.want { + t.Errorf("CheckRequiredFields() = %v, want %v", got, tt.want) + } + }) + } +} From a380a45fde6e533710802ea4c60dfedc855d2f4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Rainone?= Date: Wed, 16 Sep 2020 17:08:42 +0200 Subject: [PATCH 05/13] NewConfigFromTOML fails on unset required fields --- config.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/config.go b/config.go index 4da7e1b8..cdf3e6ca 100644 --- a/config.go +++ b/config.go @@ -291,6 +291,10 @@ func NewConfigFromToml(f io.Reader, comp Components) (*Config, error) { if err := md.PrimitiveDecode(*cfg.Input.Config, cfg.Input.DecodedConfig); err != nil { return nil, fmt.Errorf("error parsing input config: %v", err) } + + if req := CheckRequiredFields(cfg.Input.DecodedConfig); req != "" { + return nil, fmt.Errorf("input %q: %q is a required field", cfg.Input.Name, req) + } } for idx := range cfg.Filter { @@ -300,6 +304,10 @@ func NewConfigFromToml(f io.Reader, comp Components) (*Config, error) { if err := md.PrimitiveDecode(*cfg.Filter[idx].Config, cfg.Filter[idx].DecodedConfig); err != nil { return nil, fmt.Errorf("error parsing filter config: %v", err) } + + if req := CheckRequiredFields(cfg.Filter[idx].DecodedConfig); req != "" { + return nil, fmt.Errorf("filter %q: %q is a required field", cfg.Filter[idx].Name, req) + } } } @@ -308,6 +316,10 @@ func NewConfigFromToml(f io.Reader, comp Components) (*Config, error) { if err := md.PrimitiveDecode(*cfg.Output.Config, cfg.Output.DecodedConfig); err != nil { return nil, fmt.Errorf("error parsing output config: %v", err) } + + if req := CheckRequiredFields(cfg.Output.DecodedConfig); req != "" { + return nil, fmt.Errorf("output %q: %q is a required field", cfg.Output.Name, req) + } } if cfg.Upload.Name != "" { @@ -316,6 +328,10 @@ func NewConfigFromToml(f io.Reader, comp Components) (*Config, error) { if err := md.PrimitiveDecode(*cfg.Upload.Config, cfg.Upload.DecodedConfig); err != nil { return nil, fmt.Errorf("error parsing upload config: %v", err) } + + if req := CheckRequiredFields(cfg.Upload.DecodedConfig); req != "" { + return nil, fmt.Errorf("upload %q: %q is a required field", cfg.Upload.Name, req) + } } } @@ -325,6 +341,10 @@ func NewConfigFromToml(f io.Reader, comp Components) (*Config, error) { if err := md.PrimitiveDecode(*cfg.Metrics.Config, cfg.Metrics.DecodedConfig); err != nil { return nil, fmt.Errorf("error parsing metrics config: %v", err) } + + if req := CheckRequiredFields(cfg.Metrics.DecodedConfig); req != "" { + return nil, fmt.Errorf("metrics %q: %q is a required field", cfg.Metrics.Name, req) + } } } From 80d1cef91d212a29d91eb7d78c5875aa400829b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Rainone?= Date: Thu, 17 Sep 2020 12:24:43 +0200 Subject: [PATCH 06/13] Use black box testing for some of the config tests --- config_api_test.go | 119 +++++++++++++++++++++++++++++++++++++++++++++ config_test.go | 112 ------------------------------------------ 2 files changed, 119 insertions(+), 112 deletions(-) create mode 100644 config_api_test.go diff --git a/config_api_test.go b/config_api_test.go new file mode 100644 index 00000000..3bd4dee8 --- /dev/null +++ b/config_api_test.go @@ -0,0 +1,119 @@ +package baker_test + +import ( + "reflect" + "testing" + + "github.com/AdRoll/baker" +) + +func TestRequiredFields(t *testing.T) { + type ( + test1 struct { + Name string + Value string `help:"field value" required:"false"` + } + + test2 struct { + Name string + Value string `help:"field value" required:"true"` + } + + test3 struct { + Name string `required:"true"` + Value string `help:"field value" required:"true"` + } + ) + + tests := []struct { + name string + cfg interface{} + want []string + }{ + { + name: "no required fields", + cfg: &test1{}, + want: nil, + }, + { + name: "one required field", + cfg: &test2{}, + want: []string{"Value"}, + }, + { + name: "all required fields", + cfg: &test3{}, + want: []string{"Name", "Value"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := baker.RequiredFields(tt.cfg); !reflect.DeepEqual(got, tt.want) { + t.Errorf("RequiredFields() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestCheckRequiredFields(t *testing.T) { + type ( + test1 struct { + Name string + Value string `help:"field value" required:"false"` + } + + test2 struct { + Name string + Value string `help:"field value" required:"true"` + } + + test3 struct { + Name string `required:"true"` + Value string `help:"field value" required:"true"` + } + ) + + tests := []struct { + name string + val interface{} + want string + }{ + { + name: "no required fields", + val: &test1{}, + want: "", + }, + { + name: "one missing required field ", + val: &test2{Name: "name", Value: ""}, + want: "Value", + }, + { + name: "one present required field ", + val: &test2{Name: "name", Value: "value"}, + want: "", + }, + { + name: "all required fields and all are missing", + val: &test3{}, + want: "Name", + }, + { + name: "all required fields but the first missing", + val: &test3{Value: "value"}, + want: "Name", + }, + { + name: "all required fields and all are present", + val: &test3{Name: "name", Value: "value"}, + want: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := baker.CheckRequiredFields(tt.val); got != tt.want { + t.Errorf("CheckRequiredFields() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/config_test.go b/config_test.go index 79610e0d..19e81a13 100644 --- a/config_test.go +++ b/config_test.go @@ -2,7 +2,6 @@ package baker import ( "io/ioutil" - "reflect" "strings" "testing" ) @@ -102,114 +101,3 @@ func TestEnvVarBaseReplace(t *testing.T) { t.Fatalf("wrong toml: %s", string(buf)) } } - -func TestRequiredFields(t *testing.T) { - type ( - test1 struct { - Name string - Value string `help:"field value" required:"false"` - } - - test2 struct { - Name string - Value string `help:"field value" required:"true"` - } - - test3 struct { - Name string `required:"true"` - Value string `help:"field value" required:"true"` - } - ) - - tests := []struct { - name string - cfg interface{} - want []string - }{ - { - name: "no required fields", - cfg: &test1{}, - want: nil, - }, - { - name: "one required field", - cfg: &test2{}, - want: []string{"Value"}, - }, - { - name: "all required fields", - cfg: &test3{}, - want: []string{"Name", "Value"}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := RequiredFields(tt.cfg); !reflect.DeepEqual(got, tt.want) { - t.Errorf("RequiredFields() = %v, want %v", got, tt.want) - } - }) - } -} - -func TestCheckRequiredFields(t *testing.T) { - type ( - test1 struct { - Name string - Value string `help:"field value" required:"false"` - } - - test2 struct { - Name string - Value string `help:"field value" required:"true"` - } - - test3 struct { - Name string `required:"true"` - Value string `help:"field value" required:"true"` - } - ) - - tests := []struct { - name string - val interface{} - want string - }{ - { - name: "no required fields", - val: &test1{}, - want: "", - }, - { - name: "one missing required field ", - val: &test2{Name: "name", Value: ""}, - want: "Value", - }, - { - name: "one present required field ", - val: &test2{Name: "name", Value: "value"}, - want: "", - }, - { - name: "all required fields and all are missing", - val: &test3{}, - want: "Name", - }, - { - name: "all required fields but the first missing", - val: &test3{Value: "value"}, - want: "Name", - }, - { - name: "all required fields and all are present", - val: &test3{Name: "name", Value: "value"}, - want: "", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := CheckRequiredFields(tt.val); got != tt.want { - t.Errorf("CheckRequiredFields() = %v, want %v", got, tt.want) - } - }) - } -} From 8c2e171893101c8f17bbc299629db85771671ade Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Rainone?= Date: Thu, 17 Sep 2020 12:39:30 +0200 Subject: [PATCH 07/13] Add ErrorRequiredField error type --- config.go | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/config.go b/config.go index cdf3e6ca..c6a732d8 100644 --- a/config.go +++ b/config.go @@ -293,7 +293,7 @@ func NewConfigFromToml(f io.Reader, comp Components) (*Config, error) { } if req := CheckRequiredFields(cfg.Input.DecodedConfig); req != "" { - return nil, fmt.Errorf("input %q: %q is a required field", cfg.Input.Name, req) + return nil, fmt.Errorf("input: %w", ErrorRequiredField{cfg.Input.Name, req}) } } @@ -306,7 +306,7 @@ func NewConfigFromToml(f io.Reader, comp Components) (*Config, error) { } if req := CheckRequiredFields(cfg.Filter[idx].DecodedConfig); req != "" { - return nil, fmt.Errorf("filter %q: %q is a required field", cfg.Filter[idx].Name, req) + return nil, fmt.Errorf("filter: %w", ErrorRequiredField{cfg.Filter[idx].Name, req}) } } } @@ -318,7 +318,7 @@ func NewConfigFromToml(f io.Reader, comp Components) (*Config, error) { } if req := CheckRequiredFields(cfg.Output.DecodedConfig); req != "" { - return nil, fmt.Errorf("output %q: %q is a required field", cfg.Output.Name, req) + return nil, fmt.Errorf("output: %w", ErrorRequiredField{cfg.Output.Name, req}) } } @@ -330,7 +330,7 @@ func NewConfigFromToml(f io.Reader, comp Components) (*Config, error) { } if req := CheckRequiredFields(cfg.Upload.DecodedConfig); req != "" { - return nil, fmt.Errorf("upload %q: %q is a required field", cfg.Upload.Name, req) + return nil, fmt.Errorf("upload: %w", ErrorRequiredField{cfg.Upload.Name, req}) } } } @@ -343,7 +343,7 @@ func NewConfigFromToml(f io.Reader, comp Components) (*Config, error) { } if req := CheckRequiredFields(cfg.Metrics.DecodedConfig); req != "" { - return nil, fmt.Errorf("metrics %q: %q is a required field", cfg.Metrics.Name, req) + return nil, fmt.Errorf("metrics: %w", ErrorRequiredField{cfg.Metrics.Name, req}) } } } @@ -430,3 +430,14 @@ func CheckRequiredFields(val interface{}) string { return "" } + +// ErrorRequiredField describes the absence of a required field +// in a component configuration. +type ErrorRequiredField struct { + Component string // Component is the component name + Field string // Field is the name of the missing field +} + +func (e ErrorRequiredField) Error() string { + return fmt.Sprintf("%q, %q is a required field", e.Component, e.Field) +} From d04e513a08f00069101fe8e307709aec34afc846 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Rainone?= Date: Thu, 17 Sep 2020 13:09:48 +0200 Subject: [PATCH 08/13] Add required field test with NewConfigFromTOML --- config_api_test.go | 51 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/config_api_test.go b/config_api_test.go index 3bd4dee8..d4532131 100644 --- a/config_api_test.go +++ b/config_api_test.go @@ -1,10 +1,14 @@ package baker_test import ( + "errors" "reflect" + "strings" "testing" "github.com/AdRoll/baker" + "github.com/AdRoll/baker/filter/filtertest" + "github.com/AdRoll/baker/input" ) func TestRequiredFields(t *testing.T) { @@ -117,3 +121,50 @@ func TestCheckRequiredFields(t *testing.T) { }) } } + +func TestNewConfigFromTOMLRequiredField(t *testing.T) { + type dummyConfig struct { + Param1 string + Param2 string `required:"true"` + } + var dummyDesc = baker.OutputDesc{ + Name: "Dummy", + New: func(baker.OutputParams) (baker.Output, error) { return nil, nil }, + Config: &dummyConfig{}, + } + + toml := ` +[input] +name = "List" + +[input.config] +files=["testdata/input.csv.zst"] + +[output] +name = "Dummy" +procs=1 + [output.config] + param1="this parameter is set" + #param2="this parameter is not set" +` + + components := baker.Components{ + Inputs: []baker.InputDesc{input.ListDesc}, + Filters: []baker.FilterDesc{filtertest.PassThroughDesc}, + Outputs: []baker.OutputDesc{dummyDesc}, + } + + _, err := baker.NewConfigFromToml(strings.NewReader(toml), components) + if err == nil { + t.Fatal("expected an error") + } + + var errReq baker.ErrorRequiredField + if !errors.As(err, &errReq) { + t.Fatalf("got %q, want a ErrorRequiredField", err) + } + + if errReq.Component != "Dummy" || errReq.Field != "Param2" { + t.Errorf("got component=%q field=%q, want component=%q field=%q", errReq.Component, errReq.Field, "Dummy", "Param2") + } +} From 097c99ba89c17deb794b53d98af6dc1667fdc7ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Rainone?= Date: Thu, 17 Sep 2020 17:24:23 +0200 Subject: [PATCH 09/13] Refactor config decoding+check from NewConfigFromTOML --- config.go | 98 ++++++++++++++++++++++++++-------------------- config_api_test.go | 4 +- 2 files changed, 57 insertions(+), 45 deletions(-) diff --git a/config.go b/config.go index c6a732d8..0248ab65 100644 --- a/config.go +++ b/config.go @@ -211,6 +211,49 @@ func replaceEnvVars(f io.Reader, mapper func(string) string) (io.Reader, error) return strings.NewReader(os.Expand(buf.String(), mapper)), nil } +func decodeAndCheckConfig(md toml.MetaData, compCfg interface{}) error { + var ( + cfg *toml.Primitive // config + dcfg interface{} // decoded config + name string // component name + typ string // component type + ) + + switch t := compCfg.(type) { + case ConfigInput: + cfg, dcfg = t.Config, t.DecodedConfig + name, typ = t.Name, "input" + case ConfigFilter: + cfg, dcfg = t.Config, t.DecodedConfig + name, typ = t.Name, "filter" + case ConfigOutput: + cfg, dcfg = t.Config, t.DecodedConfig + name, typ = t.Name, "output" + case ConfigUpload: + cfg, dcfg = t.Config, t.DecodedConfig + name, typ = t.Name, "upload" + case ConfigMetrics: + cfg, dcfg = t.Config, t.DecodedConfig + name, typ = t.Name, "metrics" + default: + panic(fmt.Sprintf("unexpected type %#v", cfg)) + } + + if cfg == nil { + return nil + } + + if err := md.PrimitiveDecode(*cfg, dcfg); err != nil { + return fmt.Errorf("%s %q: error parsing config: %v", typ, name, err) + } + + if req := CheckRequiredFields(dcfg); req != "" { + return fmt.Errorf("%s %q: %w", typ, name, ErrorRequiredField{req}) + } + + return nil +} + // NewConfigFromToml creates a Config from a reader reading from a TOML // configuration. comp describes all the existing components. func NewConfigFromToml(f io.Reader, comp Components) (*Config, error) { @@ -287,64 +330,34 @@ func NewConfigFromToml(f io.Reader, comp Components) (*Config, error) { // Copy custom configuration structure, to prepare for re-reading cfg.Input.DecodedConfig = cfg.Input.desc.Config - if cfg.Input.Config != nil { - if err := md.PrimitiveDecode(*cfg.Input.Config, cfg.Input.DecodedConfig); err != nil { - return nil, fmt.Errorf("error parsing input config: %v", err) - } - - if req := CheckRequiredFields(cfg.Input.DecodedConfig); req != "" { - return nil, fmt.Errorf("input: %w", ErrorRequiredField{cfg.Input.Name, req}) - } + if err := decodeAndCheckConfig(md, cfg.Input); err != nil { + return nil, err } for idx := range cfg.Filter { // Clone the configuration object to allow the use of multiple instances of the same filter cfg.Filter[idx].DecodedConfig = cloneConfig(cfg.Filter[idx].desc.Config) - if cfg.Filter[idx].Config != nil { - if err := md.PrimitiveDecode(*cfg.Filter[idx].Config, cfg.Filter[idx].DecodedConfig); err != nil { - return nil, fmt.Errorf("error parsing filter config: %v", err) - } - - if req := CheckRequiredFields(cfg.Filter[idx].DecodedConfig); req != "" { - return nil, fmt.Errorf("filter: %w", ErrorRequiredField{cfg.Filter[idx].Name, req}) - } + if err := decodeAndCheckConfig(md, cfg.Filter[idx]); err != nil { + return nil, err } } cfg.Output.DecodedConfig = cfg.Output.desc.Config - if cfg.Output.Config != nil { - if err := md.PrimitiveDecode(*cfg.Output.Config, cfg.Output.DecodedConfig); err != nil { - return nil, fmt.Errorf("error parsing output config: %v", err) - } - - if req := CheckRequiredFields(cfg.Output.DecodedConfig); req != "" { - return nil, fmt.Errorf("output: %w", ErrorRequiredField{cfg.Output.Name, req}) - } + if err := decodeAndCheckConfig(md, cfg.Output); err != nil { + return nil, err } if cfg.Upload.Name != "" { cfg.Upload.DecodedConfig = cfg.Upload.desc.Config - if cfg.Upload.Config != nil { - if err := md.PrimitiveDecode(*cfg.Upload.Config, cfg.Upload.DecodedConfig); err != nil { - return nil, fmt.Errorf("error parsing upload config: %v", err) - } - - if req := CheckRequiredFields(cfg.Upload.DecodedConfig); req != "" { - return nil, fmt.Errorf("upload: %w", ErrorRequiredField{cfg.Upload.Name, req}) - } + if err := decodeAndCheckConfig(md, cfg.Upload); err != nil { + return nil, err } } if cfg.Metrics.Name != "" { cfg.Metrics.DecodedConfig = cfg.Metrics.desc.Config - if cfg.Metrics.Config != nil { - if err := md.PrimitiveDecode(*cfg.Metrics.Config, cfg.Metrics.DecodedConfig); err != nil { - return nil, fmt.Errorf("error parsing metrics config: %v", err) - } - - if req := CheckRequiredFields(cfg.Metrics.DecodedConfig); req != "" { - return nil, fmt.Errorf("metrics: %w", ErrorRequiredField{cfg.Metrics.Name, req}) - } + if err := decodeAndCheckConfig(md, cfg.Metrics); err != nil { + return nil, err } } @@ -434,10 +447,9 @@ func CheckRequiredFields(val interface{}) string { // ErrorRequiredField describes the absence of a required field // in a component configuration. type ErrorRequiredField struct { - Component string // Component is the component name - Field string // Field is the name of the missing field + Field string // Field is the name of the missing field } func (e ErrorRequiredField) Error() string { - return fmt.Sprintf("%q, %q is a required field", e.Component, e.Field) + return fmt.Sprintf("%q is a required field", e.Field) } diff --git a/config_api_test.go b/config_api_test.go index d4532131..a139d06c 100644 --- a/config_api_test.go +++ b/config_api_test.go @@ -164,7 +164,7 @@ procs=1 t.Fatalf("got %q, want a ErrorRequiredField", err) } - if errReq.Component != "Dummy" || errReq.Field != "Param2" { - t.Errorf("got component=%q field=%q, want component=%q field=%q", errReq.Component, errReq.Field, "Dummy", "Param2") + if errReq.Field != "Param2" { + t.Errorf("got field=%q, want field=%q", errReq.Field, "Param2") } } From 69cdd4f271e9b28cfb25ea471f189431aafa6ed8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Rainone?= Date: Thu, 17 Sep 2020 17:42:10 +0200 Subject: [PATCH 10/13] Print required column in help --- help.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/help.go b/help.go index 0215ba22..e87b2e24 100644 --- a/help.go +++ b/help.go @@ -130,11 +130,11 @@ func PrintHelp(w io.Writer, name string, comp Components) { } func dumpConfigHelp(w io.Writer, cfg interface{}) { - const sfmt = "%-18s | %-18s | %-26s | " + const sfmt = "%-18s | %-18s | %-18s | %-8s | " const sep = "----------------------------------------------------------------------------------------------------" - hpad := fmt.Sprintf(sfmt, "", "", "") - fmt.Fprintf(w, sfmt, "Name", "Type", "Default") + hpad := fmt.Sprintf(sfmt, "", "", "", "") + fmt.Fprintf(w, sfmt, "Name", "Type", "Default", "Required") fmt.Fprintf(w, "Help\n%s\n", sep) tf := reflect.TypeOf(cfg).Elem() @@ -175,9 +175,15 @@ func dumpConfigHelp(w io.Writer, cfg interface{}) { help := field.Tag.Get("help") def := field.Tag.Get("default") + req := field.Tag.Get("required") + if req == "true" { + req = "yes" + } else { + req = "no" + } - fmt.Fprintf(w, sfmt, field.Name, typ, def) - helpLines := strings.Split(wrapString(help, 40), "\n") + fmt.Fprintf(w, sfmt, field.Name, typ, def, req) + helpLines := strings.Split(wrapString(help, 60), "\n") if len(helpLines) > 0 { fmt.Fprint(w, helpLines[0], "\n") for _, h := range helpLines[1:] { From 866744a700a12ff629a6772b0a1ac685d2fc15c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Rainone?= Date: Thu, 17 Sep 2020 17:46:17 +0200 Subject: [PATCH 11/13] Update CHANGELOG.md --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7036ace1..194791e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,8 +19,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - general: replace `${KEY}` in the TOML conf with the `$KEY` env var [#24](https://github.com/AdRoll/baker/pull/24) - input: add KCL input. [#36](https://github.com/AdRoll/baker/pull/36) - filter: add RegexMatch filter. [#37](https://github.com/AdRoll/baker/pull/37) -- filter: add Concatenate filter [#33](https://github.com/AdRoll/baker/pull/33) - filter: add NotNull filter [#43](https://github.com/AdRoll/baker/pull/43) +- filter: add Concatenate filter [#28](https://github.com/AdRoll/baker/pull/33) +- Required configuration fields are now handled by Baker rather than by each component. [#41](https://github.com/AdRoll/baker/pull/41) ### Changed From 4994a35bbae940e941886e5d3f816df319d5be98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Rainone?= Date: Mon, 21 Sep 2020 12:48:49 +0200 Subject: [PATCH 12/13] Check for required fields before early exit in case of nil config --- config.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/config.go b/config.go index 0248ab65..4108830f 100644 --- a/config.go +++ b/config.go @@ -239,6 +239,10 @@ func decodeAndCheckConfig(md toml.MetaData, compCfg interface{}) error { panic(fmt.Sprintf("unexpected type %#v", cfg)) } + if req := CheckRequiredFields(dcfg); req != "" { + return fmt.Errorf("%s %q: %w", typ, name, ErrorRequiredField{req}) + } + if cfg == nil { return nil } @@ -247,10 +251,6 @@ func decodeAndCheckConfig(md toml.MetaData, compCfg interface{}) error { return fmt.Errorf("%s %q: error parsing config: %v", typ, name, err) } - if req := CheckRequiredFields(dcfg); req != "" { - return fmt.Errorf("%s %q: %w", typ, name, ErrorRequiredField{req}) - } - return nil } From 994e8fc700b675afd3e4791ca5d14b82068a04c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Rainone?= Date: Mon, 21 Sep 2020 12:50:08 +0200 Subject: [PATCH 13/13] Cosmetics --- config.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/config.go b/config.go index 4108830f..c2d1b029 100644 --- a/config.go +++ b/config.go @@ -424,17 +424,17 @@ func RequiredFields(cfg interface{}) []string { } // CheckRequiredFields checks that all fields that are tagged as required in -// val's type have actually been set to a value other than the field type zero +// cfg's type have actually been set to a value other than the field type zero // value. If not CheckRequiredFields returns the name of the first required // field that is not set, or, it returns an empty string if all required fields // are set of the struct doesn't have any required fields (or any fields at all). // // CheckRequiredFields doesn't support struct embedding other structs. -func CheckRequiredFields(val interface{}) string { - fields := RequiredFields(val) +func CheckRequiredFields(cfg interface{}) string { + fields := RequiredFields(cfg) for _, name := range fields { - rv := reflect.ValueOf(val).Elem() + rv := reflect.ValueOf(cfg).Elem() fv := rv.FieldByName(name) if fv.IsZero() { return name