Validation support for strings, arrays, numbers #329

Porges · 2020-11-18T23:10:09Z

Ok, this got a bit bigger than expected:

Add ValidatedType which adds support for validations.
Handle specially the ^.*/default$ validation used by Storage for names; disable AzureName when detected.
- To do in future PR: make this more general.
Add SetAzureName function instead of writing the “setter” code inline in the from-ARM conversion, which simplifies things somewhat.
Simplify PropertyDefinition by removing general validation support; properties only have Required as a validation.
Add a Defaulter webhook to copy Name into AzureName (when required), this has a marker comment for controller-gen which outputs the webhook configuration files.
Set up webhooks for integration tests.

Porges · 2020-11-19T04:15:07Z

Taskfile.yml

    - if [ -d "./config/crd/bases" ]; then find "./config/crd/bases" -type f -delete; fi
+    - controller-gen object:headerFile=../boilerplate.go.txt paths="./..." || true


TO FIX: controller-gen emitting bad code

Porges · 2020-11-22T22:37:45Z

hack/generated/controllers/controller_test.go

@@ -123,10 +122,7 @@ func StorageAccount_BlobServices_CRUD(t *testing.T, testContext testcommon.KubeP
 	g := NewGomegaWithT(t)

 	blobService := &storage.StorageAccountsBlobService{
-		ObjectMeta: ctrl.ObjectMeta{
-			Name:      "default", // MUST be 'default'


this is the thing I’ve been working towards 😀

(I know @babbageclunk likes stuff like this 😉)

It's like when they say the title of the movie in the movie.

.devcontainer/install-dependencies.sh

Taskfile.yml

coveralls · 2020-11-23T23:32:53Z

Coverage increased (+13.02%) to 65.353% when pulling 34dbb99 on validation-support into 1d9651a on master.

Taskfile.yml

hack/generator/pkg/astmodel/kubebuilder_validations.go

matthchr · 2020-11-24T01:01:08Z

Taskfile.yml

+    dir: "{{.CONTROLLER_ROOT}}"
+    deps: [generate-crds]
+    cmds:
+    # the webhook configuration file, as it stands, is too big to load into k8s 😊


Should we open an issue with kubebuilder on this topic? Presumably the issue is that the yaml file is too big but if it were split up it'd be ok?

Or maybe there already is one? Link it here if so?

It might actually be small enough now; part of the earlier problem was webhook definitions were being generated for the storage types.

A few other thoughts:

Yes, I think controller-gen should generate multiple files

We should be able to generate fewer webhooks that apply to more types.

Taskfile.yml

hack/generated/pkg/genruntime/base_types.go

hack/generated/pkg/testcommon/kube_test_context_envtest.go

hack/generator/pkg/codegen/testdata/EnumNames/Single_valued_enum_name.golden

hack/generator/pkg/astmodel/kubernetes_resource_interface.go

matthchr · 2020-11-26T01:02:28Z

hack/generator/pkg/astmodel/property_definition.go

@@ -22,7 +22,7 @@ type PropertyDefinition struct {
 	propertyName PropertyName
 	propertyType Type
 	description  string
-	validations  []Validation
+	isRequired   bool


Isn't this about the required validation?

Why have this here then?

Oh it's about the validation and the Optional-ness of the type, ok.

the requiredness is a property of the property, not of the type of the property (this is all a bit conflated by us because we tie this to optionalness via MakeRequired/MakeOptional)

hack/generator/pkg/astmodel/property_definition_test.go

theunrepentantgeek

Some minor feedback, nothing too material.

hack/generator/pkg/astmodel/type_visitor.go

.../generator/pkg/codegen/testdata/ArmResource/Arm_test_dependent_resource_and_ownership.golden

Porges requested review from babbageclunk, devigned, matthchr and theunrepentantgeek as code owners November 18, 2020 23:10

Base automatically changed from the-resource-named-default to master November 19, 2020 04:11

Porges force-pushed the validation-support branch 2 times, most recently from 440ceef to ff50ac7 Compare November 19, 2020 04:14

Porges commented Nov 19, 2020

View reviewed changes

Porges force-pushed the validation-support branch 2 times, most recently from 2e50efe to c01d898 Compare November 22, 2020 22:35

Porges changed the base branch from master to dst November 22, 2020 22:36

Porges commented Nov 22, 2020

View reviewed changes

Porges force-pushed the dst branch from e56cf90 to 926e0b7 Compare November 23, 2020 02:26

Porges force-pushed the validation-support branch 2 times, most recently from 7902a4a to b341b50 Compare November 23, 2020 04:05

Base automatically changed from dst to master November 23, 2020 19:47

Porges force-pushed the validation-support branch from b341b50 to b56a371 Compare November 23, 2020 19:49

Porges commented Nov 23, 2020

View reviewed changes

.devcontainer/install-dependencies.sh Show resolved Hide resolved

Porges commented Nov 23, 2020

View reviewed changes

Taskfile.yml Outdated Show resolved Hide resolved

Porges commented Nov 24, 2020

View reviewed changes

Taskfile.yml Outdated Show resolved Hide resolved

Porges force-pushed the validation-support branch from 16d020a to 9dcbb87 Compare November 24, 2020 20:05

Porges added 7 commits November 24, 2020 20:34

Handle validations!

48c2b2c

Change how properties have 'required' validation

0a53c6c

Add SetAzureName and webhook for AzureName defaulting

ee499dc

Remove thing about "must be default" because it's fixed!

52e6214

Add required sideEffects setting

8cb396e

Tweaks to field rendering

465433a

Configuration for webhooks

801d0f4

Update new golden files after rebase

ce697e2

Porges force-pushed the validation-support branch from 9dcbb87 to ce697e2 Compare November 24, 2020 20:35

Porges commented Nov 24, 2020

View reviewed changes

hack/generator/pkg/astmodel/kubebuilder_validations.go Show resolved Hide resolved

matthchr reviewed Nov 26, 2020

View reviewed changes

Porges added 10 commits December 1, 2020 22:19

Add link to issue

3eaf167

Expand comment

69702ec

Fix up const name

cd48289

Fix bad error 😊

73c7dbe

Strip validations from ARM types

438ef9c

Fix names of property tests

d121c76

Lint fix

421ddaa

Merge branch 'master' into validation-support

4f6fd72

Implement test case generation for validated types

3d67b9c

Support for validated types in testcase generation

2191426

Porges mentioned this pull request Dec 2, 2020

Figure out how best to handle resources that must be named default in Azure #324

Closed

Fixup validation conversion

a5d568f

Porges self-assigned this Dec 3, 2020

Porges added 9 commits December 6, 2020 21:46

Reformat very long line

67b67d1

Use astbuilder helper

478a9af

Introduce DeclarationContext

2abe786

Fix go.mod for generated tests

fddd910

Fix gopter codegen

bb0550b

Merge branch 'master' into validation-support

30d3bfd

Document resource interface functions

7a3d305

Merge branch 'master' into validation-support

fc441d0

Post-merge fixup

34dbb99

theunrepentantgeek approved these changes Dec 17, 2020

View reviewed changes

hack/generator/pkg/astmodel/type_visitor.go Show resolved Hide resolved

hack/generator/pkg/astmodel/type_visitor.go Show resolved Hide resolved

.../generator/pkg/codegen/testdata/ArmResource/Arm_test_dependent_resource_and_ownership.golden Show resolved Hide resolved

Porges merged commit 6842fcf into master Dec 18, 2020

Porges deleted the validation-support branch December 18, 2020 00:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validation support for strings, arrays, numbers #329

Validation support for strings, arrays, numbers #329

Porges commented Nov 18, 2020 •

edited

Loading

Porges Nov 19, 2020 •

edited

Loading

Porges Nov 22, 2020

Porges Nov 24, 2020

babbageclunk Nov 25, 2020

coveralls commented Nov 23, 2020 •

edited

Loading

matthchr Nov 24, 2020

matthchr Nov 24, 2020

Porges Dec 1, 2020 •

edited

Loading

Porges Dec 6, 2020

matthchr Nov 26, 2020

matthchr Nov 26, 2020

Porges Dec 6, 2020

theunrepentantgeek left a comment

		- if [ -d "./config/crd/bases" ]; then find "./config/crd/bases" -type f -delete; fi
		- controller-gen object:headerFile=../boilerplate.go.txt paths="./..." \|\| true

Validation support for strings, arrays, numbers #329

Validation support for strings, arrays, numbers #329

Conversation

Porges commented Nov 18, 2020 • edited Loading

Porges Nov 19, 2020 • edited Loading

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

coveralls commented Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Porges Dec 1, 2020 • edited Loading

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

theunrepentantgeek left a comment

Choose a reason for hiding this comment

Porges commented Nov 18, 2020 •

edited

Loading

Porges Nov 19, 2020 •

edited

Loading

coveralls commented Nov 23, 2020 •

edited

Loading

Porges Dec 1, 2020 •

edited

Loading