test(manifest): add integ test making sure all structs implement Validate #2895

iamhopaul123 · 2021-10-04T23:06:02Z

This PR

Add integ test to make sure every manifest struct or advanced type implements Validate().
Fix a bug that network.placement should not be required even if network is not empty.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

…date

efekarakus · 2021-10-05T17:42:03Z

internal/pkg/manifest/validate.go

@@ -44,7 +44,26 @@ var (
 )

 // Validate returns nil if LoadBalancedWebService is configured correctly.
-func (l *LoadBalancedWebService) Validate() error {
+func (l LoadBalancedWebService) Validate() error {


Is there a reason why we switched the receivers from pointers to values?

Because it won't work otherwise since Validate() would be the method of * LoadBalancedWebService. Also I feel it doesn't make the code much cleaner with pointer receiver.

efekarakus · 2021-10-05T20:18:53Z

internal/pkg/manifest/validate.go

@@ -44,7 +44,26 @@ var (
 )

 // Validate returns nil if LoadBalancedWebService is configured correctly.
-func (l *LoadBalancedWebService) Validate() error {
+func (l LoadBalancedWebService) Validate() error {
+	var err error


nit: can we remove the variable with:

if err := l...Validate();

Hmm wouldn't this avoid defining err repetitively?

efekarakus · 2021-10-05T20:27:04Z

internal/pkg/manifest/validate.go

-		return nil
-	}
-	if val := int(*p); val < 0 || val > 100 {
+func (p Percentage) Validate() error {


ok so by moving it to value receivers we're assuming the callers will do the nil-check from now on right?

yeah that's right.

efekarakus · 2021-10-05T20:34:44Z

internal/pkg/manifest/validate_integration_test.go

+	var val validator
+	validatorType := reflect.TypeOf(&val).Elem()
+	// template.Parser is not a manifest struct.
+	var tpl template.Parser
+	templaterType := reflect.TypeOf(&tpl).Elem()
+	if !typ.Implements(templaterType) && !typ.Implements(validatorType) {
+		return fmt.Errorf(`%v does not implement "Validate()"`, typ)
+	}


I wonder if instead of ignoring template.Parser as a type we can check if the field has a "yaml" struct tag and if so check it? https://stackoverflow.com/a/23507821/1201381

What do you think?

internal/pkg/manifest/validate_integration_test.go

Lou1415926

Two questions 🤚🏼

internal/pkg/manifest/validate_integration_test.go

Lou1415926 · 2021-10-05T22:50:27Z

internal/pkg/manifest/validate_integration_test.go

+	}
+	for i := 0; i < typ.NumField(); i++ {
+		// Skip if it is not a manifest yaml field.
+		if typ.Field(i).Tag.Get("yaml") == "" {


What about fields such as HealthCheckArgs that does not have a tag?

That's true. I guess I'd rather revert this change @efekarakus

😭 I'm just not a fan because it feels so fragile otherwise.
As a requirement, it makes sense to me to read "any field used during unmarshalling YAML should be validated", but now we lose that nice definition.

Isn't it good enough to have the Validate method on HealthCheckArgsOrString ? since that already will call internally Validate on any of its exclusive subfields. I'd rather have us keep typ.Field(i).Tag.Get("yaml") == ""

since that already will call internally Validate on any of its exclusive subfields.

Problem is this is not always guaranteed...other people could forget to do so, which is the whole point to have this test

Lou1415926 · 2021-10-06T19:32:27Z

internal/pkg/manifest/validate_integration_test.go

+	}
+	// For slice and map, validate individual member.
+	if typ.Kind() == reflect.Array || typ.Kind() == reflect.Slice {
+		for i := 0; i < v.Len(); i++ {


Just a thought 💭 .

Since isValid validates whether a type is implementing an interface as expected, should we use t reflect.Type as its parameter, instead of v reflect.Vlaue?

Then here we can use elemTyp := typ.Elem() to retrieve the element type of the array/slice, and then call isValid(elemTyp).

since we are now skipping private fields, we have to use value instead of type

but i guess you are right. we only need to check the first element since they are all the same

discussed offline now use structfield.IsExported() to check so that we can use type as the parameter.

Lou1415926 · 2021-10-06T19:33:59Z

internal/pkg/manifest/validate_integration_test.go

 	}
+	if typ.Kind() == reflect.Map {
+		for _, k := range v.MapKeys() {


And also use typ.Key() and typ.Elem() here.

Lou1415926 · 2021-10-06T20:41:53Z

internal/pkg/manifest/validate_test.go

-		})
-	}
-}
+// func TestPlatformString_Validate(t *testing.T) {


Is this intentionally commented out?

Lou1415926 · 2021-10-06T21:16:25Z

internal/pkg/manifest/validate_integration_test.go

+	for _, k := range basicKinds {
+		types = append(types, k.String())
+	}
+	types = append(types, reflect.TypeOf(map[string]string{}).String())


What about this:

In isValid, we detect the map's key and value types, and recursively call isValid if they are of non-basic types.

Therefore, we don't need special treatment for map[string]string - since string and string are basic, they won't get called on isValid.

Same for slice.

this is what i was doing! i forgot to remove those

Lou1415926

hooray!

…date (aws#2895)  This PR 1. Add integ test to make sure every manifest struct or advanced type implements `Validate()`. 2. Fix a bug that `network.placement` should not be required even if `network` is not empty.  By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

iamhopaul123 requested a review from a team as a code owner October 4, 2021 23:06

iamhopaul123 requested review from efekarakus and removed request for a team October 4, 2021 23:06

test(manifest): add integ test making sure all structs implement Vali…

45bd195

…date

iamhopaul123 force-pushed the manifest-validation-audit branch from 02e9895 to 45bd195 Compare October 4, 2021 23:13

efekarakus reviewed Oct 5, 2021

View reviewed changes

internal/pkg/manifest/validate_integration_test.go Outdated Show resolved Hide resolved

iamhopaul123 force-pushed the manifest-validation-audit branch from 06e827a to e5e75ef Compare October 5, 2021 21:12

efekarakus approved these changes Oct 5, 2021

View reviewed changes

Lou1415926 reviewed Oct 5, 2021

View reviewed changes

iamhopaul123 force-pushed the manifest-validation-audit branch from e5e75ef to 45bd195 Compare October 5, 2021 23:05

Make advanced type required for Validate

a13333b

iamhopaul123 requested a review from efekarakus October 6, 2021 00:31

Lou1415926 reviewed Oct 6, 2021

View reviewed changes

Skip any private field

4834a9e

iamhopaul123 force-pushed the manifest-validation-audit branch from 13094ba to 4834a9e Compare October 6, 2021 21:49

Use type instead of value

703d597

iamhopaul123 force-pushed the manifest-validation-audit branch from d2675f0 to 703d597 Compare October 6, 2021 22:05

Update comment msg

c7b1bb2

Lou1415926 approved these changes Oct 6, 2021

View reviewed changes

Merge branch 'mainline' into manifest-validation-audit

dfd0028

mergify bot merged commit 46743e7 into aws:mainline Oct 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(manifest): add integ test making sure all structs implement Validate #2895

test(manifest): add integ test making sure all structs implement Validate #2895

iamhopaul123 commented Oct 4, 2021

efekarakus Oct 5, 2021

iamhopaul123 Oct 5, 2021

efekarakus Oct 5, 2021

iamhopaul123 Oct 5, 2021

efekarakus Oct 5, 2021

iamhopaul123 Oct 5, 2021

efekarakus Oct 5, 2021

iamhopaul123 Oct 5, 2021

Lou1415926 left a comment

Lou1415926 Oct 5, 2021

iamhopaul123 Oct 5, 2021

efekarakus Oct 5, 2021

iamhopaul123 Oct 6, 2021

Lou1415926 Oct 6, 2021

iamhopaul123 Oct 6, 2021

iamhopaul123 Oct 6, 2021

iamhopaul123 Oct 6, 2021

Lou1415926 Oct 6, 2021

Lou1415926 Oct 6, 2021

iamhopaul123 Oct 6, 2021

Lou1415926 Oct 6, 2021

iamhopaul123 Oct 6, 2021

Lou1415926 left a comment

test(manifest): add integ test making sure all structs implement Validate #2895

test(manifest): add integ test making sure all structs implement Validate #2895

Conversation

iamhopaul123 commented Oct 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lou1415926 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lou1415926 left a comment

Choose a reason for hiding this comment