Updated object specs to pass tests #27

runyontr · 2018-12-04T05:11:23Z

$ go test ./pkg/apis/maestro/v1alpha1 -v
=== RUN   TestStorageFramework
--- PASS: TestStorageFramework (0.02s)
=== RUN   TestStorageFrameworkVersion
--- PASS: TestStorageFrameworkVersion (0.02s)
=== RUN   TestStorageInstance
--- PASS: TestStorageInstance (0.02s)
=== RUN   TestStoragePlanExecution
--- PASS: TestStoragePlanExecution (0.03s)
PASS
ok  	github.com/kubernetes-sigs/kubebuilder-maestro/pkg/apis/maestro/v1alpha1```

Tests still fail in the controller packages since the logic in them havent been changed from the default (look for a deployment when an object is created).

runyontr · 2018-12-05T02:38:50Z

@fabianbaier did this fix what you were asking for?

fabianbaier · 2018-12-07T22:34:35Z

pkg/apis/maestro/v1alpha1/frameworkversion_types.go

 }

 //Phase specifies a list of steps that contain Kubernetes objects.
 type Phase struct {
-	Name     string   `json:"name"`
-	Strategy Ordering `json:"strategy"`
+	Name     string   `json:"name" validate:"required,gt=0"`     // makes field mandatory and checks if set and non empty


should we make this a pointer? *string

fabianbaier · 2018-12-07T22:35:40Z

pkg/apis/maestro/v1alpha1/frameworkversion_types.go

 	//Phases maps a phase name to a Phase object
-	Phases []Phase `json:"phases"`
+	Phases []Phase `json:"phases" validate:"gt=0,dive"` // makes field mandatory and checks if its gt 0


[]*Phase ?

fabianbaier · 2018-12-07T22:36:03Z

pkg/apis/maestro/v1alpha1/frameworkversion_types.go

 	//Steps maps a step name to a list of mustached kubernetes objects stored as a string
-	Steps []Step `json:"steps"`
+	Steps []Step `json:"steps" validate:"gt=0,dive"` // makes field mandatory and checks if its gt 0


[]*Steps ?

fabianbaier · 2018-12-07T22:36:19Z

pkg/apis/maestro/v1alpha1/frameworkversion_types.go

 }

 type Step struct {
-	Name     string `json:"name"`
-	Mustache string `json:"mustache"`
+	Name     string `json:"name" validate:"required,gt=0"`     // makes field mandatory and checks if set and non empty


*string ?

fabianbaier · 2018-12-07T22:36:30Z

pkg/apis/maestro/v1alpha1/frameworkversion_types.go

-	Name     string `json:"name"`
-	Mustache string `json:"mustache"`
+	Name     string `json:"name" validate:"required,gt=0"`     // makes field mandatory and checks if set and non empty
+	Mustache string `json:"mustache" validate:"required,gt=0"` // makes field mandatory and checks if set and non empty


*string ?

fabianbaier · 2018-12-07T22:36:48Z

pkg/apis/maestro/v1alpha1/frameworkversion_types.go

 	//Objects will be serialized for each instance as the params and defaults
 	// are provided
-	Objects []runtime.Object `json:"-"`
+	Objects []runtime.Object `json:"-"` // no checks needed


Is this optional or mandatory?

fabianbaier

Your part looks good 👍

I pushed the logic changes for my part with some comments to look at.

fabianbaier · 2018-12-07T22:38:29Z

pkg/apis/maestro/v1alpha1/frameworkversion_types_test.go

+		"ServiceSpec.Pods[pod2].HostVolumes[].HostPath:required: cannot be <nil>",
+		"ServiceSpec.Pods[pod2].HostVolumes[].ContainerPath:required: cannot be <nil>",
+		"ServiceSpec.Plans[]:required: cannot be empty",
+		"ServiceSpec.Plans[].Strategy:required: cannot be ",


because we have no pointer we cannot tell the difference of something being set or with its zeroValue.

# Conflicts: # config/crds/kudo_v1alpha1_instance.yaml # config/crds/maestro_v1alpha1_instance.yaml # pkg/apis/kudo/v1alpha1/frameworkversion_types.go # pkg/apis/kudo/v1alpha1/planexecution_types.go

runyontr · 2019-02-15T10:32:03Z

This LGTM for now. Lets make sure we make a ticket addressing adding tests for validation for Plans, and FVs.

fabianbaier · 2019-02-19T23:13:56Z

This LGTM for now. Lets make sure we make a ticket addressing adding tests for validation for Plans, and FVs.

Awesome! Can I get your approval so I am able to merge it?

fabianbaier

lgtm

joerg84 · 2019-03-11T23:50:05Z

pkg/apis/kudo/v1alpha1/frameworkversion_types.go

 	//Phases maps a phase name to a Phase object
-	Phases []Phase `json:"phases"`
+	Phases []Phase `json:"phases" validate:"gt=0,dive"` // makes field mandatory and checks if its gt 0


Is this dive needed? It feels weird at the end of the chain?

Should that field be required?

Is this dive needed? It feels weird at the end of the chain?

Phases []Phase 'json:"phases" validate:"gt=0,dive"'

GreaterThan looks at the slice and ensures it has at least one item - it doesn't check if its an empty item or anything else with the item is wrong. Therefor we have dive.

Dive is used here to ensure that the values of the Phase struct will be validated after its validate tags. If I leave dive out here what happens it just looks at the slice, but I need it one level deeper. The test would fail as each Phaseitem in the slice wouldn't be validated anymore:

=== RUN TestFrameworkType_Validation --- FAIL: TestFrameworkType_Validation (0.00s) frameworkversion_types_test.go:408: Missed expected error: ServiceSpec.Plans[plan2].Phases[0].Name:required: cannot be empty frameworkversion_types_test.go:408: Missed expected error: ServiceSpec.Plans[plan2].Phases[0].Strategy:required: cannot be frameworkversion_types_test.go:408: Missed expected error: ServiceSpec.Plans[plan2].Phases[0].Steps:gt: cannot be [] frameworkversion_types_test.go:408: Missed expected error: ServiceSpec.Plans[plan2].Phases[1].Steps[0].Name:required: cannot be empty frameworkversion_types_test.go:408: Missed expected error: ServiceSpec.Plans[plan2].Phases[1].Steps[0].Tasks:required: cannot be [] frameworkversion_types_test.go:408: Missed expected error: ServiceSpec.Plans[plan2].Phases[1].Steps[1].Tasks[0]:gt: cannot be empty FAIL

You don't find at the end of the chain dives in the official examples because they deal already with the final string data type. E.g.

[][]string with validation tag "gt=0,dive,len=1,dive,required" // gt=0 will be applied to [] // len=1 will be applied to []string // required will be applied to string

There is no nested level in string so that is why after dive a required is applied to the string type. Without the dive tag required would just be applied to []string. While it says for validate.Struct that it will automatically validate nested structs I noticed the opposite. I filed already an issue with go-playground/validator#458 but for the desired outcome it requires us right now to use dive at the end to kick off the validation of a nested struct imo.

fabianbaier · 2019-03-14T01:19:19Z

I intended to make the validation tests more consistent. This isnt in any way an attempt to be perfect. I am sure we will find some oversight. For now the tests pass though. With f58f998 comes this gist Validation Lexicon that I used to explain the various different appearances and how to validate them. Hope this gets lgtm

Updated object specs to pass tests

40775a8

runyontr requested a review from fabianbaier December 4, 2018 05:11

gerred added kind/cleanup priority/normal needs review labels Dec 6, 2018

gerred added this to the v0.2.0 milestone Dec 6, 2018

gerred assigned runyontr and fabianbaier Dec 6, 2018

respected logic change in validation tests

2304150

fabianbaier reviewed Dec 7, 2018

View reviewed changes

fabianbaier added 2 commits February 13, 2019 14:48

Merge remote-tracking branch 'origin/master' into fix-crd-tests

a05f569

# Conflicts: # config/crds/kudo_v1alpha1_instance.yaml # config/crds/maestro_v1alpha1_instance.yaml # pkg/apis/kudo/v1alpha1/frameworkversion_types.go # pkg/apis/kudo/v1alpha1/planexecution_types.go

fixed validation test

1aa83af

fabianbaier approved these changes Feb 20, 2019

View reviewed changes

joerg84 reviewed Mar 11, 2019

View reviewed changes

addressed review and made validation tests consistent

f58f998

joerg84 approved these changes Mar 14, 2019

View reviewed changes

fabianbaier merged commit dd3ad09 into master Mar 14, 2019

fabianbaier deleted the fix-crd-tests branch March 14, 2019 03:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated object specs to pass tests #27

Updated object specs to pass tests #27

runyontr commented Dec 4, 2018

runyontr commented Dec 5, 2018

fabianbaier Dec 7, 2018

fabianbaier Dec 7, 2018

fabianbaier Dec 7, 2018

fabianbaier Dec 7, 2018

fabianbaier Dec 7, 2018

fabianbaier Dec 7, 2018

fabianbaier left a comment

fabianbaier Dec 7, 2018

runyontr commented Feb 15, 2019

fabianbaier commented Feb 19, 2019

fabianbaier left a comment

joerg84 Mar 11, 2019 •

edited

Loading

joerg84 Mar 11, 2019

fabianbaier Mar 12, 2019

fabianbaier commented Mar 14, 2019

Updated object specs to pass tests #27

Updated object specs to pass tests #27

Conversation

runyontr commented Dec 4, 2018

runyontr commented Dec 5, 2018

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

fabianbaier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runyontr commented Feb 15, 2019

fabianbaier commented Feb 19, 2019

fabianbaier left a comment

Choose a reason for hiding this comment

joerg84 Mar 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianbaier commented Mar 14, 2019

joerg84 Mar 11, 2019 •

edited

Loading