Handle required fields by arl · Pull Request #41 · AdRoll/baker

arl · 2020-09-17T15:45:19Z

❓ What

Component configuration fields can now be tagged as required:"true". If that's the case, the configuration is considered valid if and only if the field has been set to a non-zero value. This allows to handle required fields in a single place, rather than all over the place, and with a homogeneous error message.

Modification to the components configuration to set required fields as required and remove component-specific checks will be pushed in a follow-up PR

🔨 How to test

List all steps necessary;
To test this pull request.

✅ Checklists

This section contains a list of checklists for common uses, please delete the checklists that are useless for your current use case (or add another checklist if your use case isn't covered yet).

Is there unit/integration test coverage for all new and/or changed functionality added in this PR?
Have the changes in this PR been functionally tested?
Has make gofmt-write been run on the code?
Has make govet been run on the code? Has the code been fixed accordingly to the output?
Have the changes been added to the CHANGELOG.md file?
Have the steps in CONTRIBUTING.md been followed to update a Go module?

tommyblue

sounds good, check the comments

tommyblue · 2020-09-21T09:49:11Z

+		cfg, dcfg = t.Config, t.DecodedConfig
+		name, typ = t.Name, "metrics"
+	default:
+		panic(fmt.Sprintf("unexpected type %#v", cfg))


panic? shouldn't it return an error?

Getting an unexpected type here means there's a developer error which must be fixed, no way around that.
That's the typical use case for panic

tommyblue · 2020-09-21T09:50:19Z

+		panic(fmt.Sprintf("unexpected type %#v", cfg))
+	}
+
+	if cfg == nil {


uhm, does this ever happen? isn't already panicked in the switch?

uhm, does this ever happen? isn't already panicked in the switch?
yes it does. if t.Config is nil. t.Config is nil if the user didn't specify an [input.config] section in the TOML.

tommyblue · 2020-09-21T10:02:54Z

+		return nil
+	}
+
+	if err := md.PrimitiveDecode(*cfg, dcfg); err != nil {


PrimitiveDecode is deprecated, in favor of MetaData.PrimitiveDecode. Maybe it's worth updating it here

where did you see that? link?

the editor suggested that, but I now see it makes a mistake: the deprecated function is func PrimitiveDecode, not func (md *MetaData) PrimitiveDecode

tommyblue · 2020-09-21T10:09:14Z

+		return fmt.Errorf("%s %q: error parsing config: %v", typ, name, err)
+	}
+
+	if req := CheckRequiredFields(dcfg); req != "" {


req could be called fieldName?

https://github.com/golang/go/wiki/CodeReviewComments#variable-names
IMHO fieldName seems a bit redundant here.
I actually hesitated calling it just name.
Maybe just calling it f could do if I transform the next line to be:

ErrorRequiredField{Field:f}

rather than

ErrorRequiredField{req}

tommyblue · 2020-09-21T10:09:47Z

+// "required" struct struct tag must be present and set to true.
+//
+// RequiredFields doesn't support struct embedding other structs.
+func RequiredFields(cfg interface{}) []string {


is this public for testing purposes?

no. I thought this could be a good addition to the public API for users that desire to use baker as a library.
We're provided the help functions and all but we should also provide the necessary API for users to differently.
That's why I added a detailed documentation there.

tommyblue · 2020-09-21T10:10:00Z

+// 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 {


is the function public for testing purposes?

same answer as for RequiredFields

tommyblue · 2020-09-21T10:15:15Z

Oh, I forgot: I think we should also add this change to the README

arl · 2020-09-21T13:18:47Z

Oh, I forgot: I think we should also add this change to the README

At the moment there's not a clear section in the README about the configuration structures of baker components.
I think we should create one and document the new feature there.
creating a ticket for that.
@tommyblue

tommyblue

ok, just remove the line from the changelog then merge

tommyblue · 2020-09-21T14:25:22Z

 - 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)


Suggested change

- filter: add Concatenate filter [#28](https://github.com/AdRoll/baker/pull/33)

already included in the file

arl requested a review from tommyblue September 17, 2020 15:45

arl mentioned this pull request Sep 17, 2020

Set required fields #42

Merged

6 tasks

arl force-pushed the handle-required-fields branch 3 times, most recently from ea9bcca to f0f69e0 Compare September 21, 2020 08:58

tommyblue reviewed Sep 21, 2020

View reviewed changes

arl added 10 commits September 21, 2020 15:12

Move hasConfig in config.go

6301a1c

lint: don't use underscores in Go names

1cd5246

Add RequiredFields function + test

3d5ca1f

Add CheckRequiredFields + test

e6aa841

NewConfigFromTOML fails on unset required fields

a380a45

Use black box testing for some of the config tests

80d1cef

Add ErrorRequiredField error type

8c2e171

Add required field test with NewConfigFromTOML

d04e513

Refactor config decoding+check from NewConfigFromTOML

097c99b

Print required column in help

69cdd4f

arl force-pushed the handle-required-fields branch from e1b6265 to 50cb62c Compare September 21, 2020 13:13

tommyblue approved these changes Sep 21, 2020

View reviewed changes

arl added 3 commits September 21, 2020 16:33

Update CHANGELOG.md

866744a

Check for required fields before early exit in case of nil config

4994a35

Cosmetics

994e8fc

arl force-pushed the handle-required-fields branch from 50cb62c to 994e8fc Compare September 21, 2020 14:33

arl merged commit b1fa829 into main Sep 21, 2020

arl deleted the handle-required-fields branch September 21, 2020 14:44

arl mentioned this pull request Sep 21, 2020

Check required fields also for nil config sections #48

Merged

6 tasks

Conversation

arl commented Sep 17, 2020

❓ What

🔨 How to test

✅ Checklists

Uh oh!

tommyblue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tommyblue commented Sep 21, 2020

Uh oh!

arl commented Sep 21, 2020

Uh oh!

tommyblue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants