Repository: kubernetes-sigs/controller-tools
Pull request #228 (2019-06-19 16:21:51)

🐛Add FieldOnlyMarkers to AllDefinitions and update Nullable
Currently the `+optional` and `+nullable` markers don't work. I've fixed `optional` by making sure the `FieldOnlyMarkers` slice that was added in https://github.com/kubernetes-sigs/controller-tools/pull/211 actually gets added to `AllDefinitions`. In the current code, `+nullable` would actually only be recognized as `+kubebuilder:validation:Nullable=true`, so I'm updating this to be more consistent with what's expected for that marker.

However, I still can't get nullable to be generated in the output CRD, so I would appreciate some input on this

k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/controller-tools/pull/228#" title="Author self-approved">damemi</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **droot**
You can assign the PR to them by writing `/assign @droot` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fcontroller-tools).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-sigs/controller-tools/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["droot"]} -->
damemi: Want to note that in debugging this, by inspecting the schema props around here https://github.com/kubernetes-sigs/controller-tools/blob/master/pkg/crd/schema.go#L378 I do see `nullable: true` being set on fields that have it, but still not showing up in the final generated output. Also if I manually set `propSchema.Nullable = true` around that area, I *do* see nullable in the generated output. But not if I put it in a conditional (ie, checking for `field.Markers.Get("nullable")`, which returns true and sets the schema, but not in the final output)
damemi: Update: the nullable problem I saw is fixed by https://github.com/kubernetes-sigs/controller-tools/pull/226
damemi: /retest
DirectXMan12: Adding a test case to the integration test data (recently merged, in `pkg/crd/testdata`).  Otherwise lgtm.

Patch:
diff --git a/pkg/crd/markers/validation.go b/pkg/crd/markers/validation.go
index fbe19bf4..56fcb607 100644
--- a/pkg/crd/markers/validation.go
+++ b/pkg/crd/markers/validation.go
@@ -58,7 +58,6 @@ var ValidationMarkers = mustMakeAllWithPrefix("kubebuilder:validation", markers.
 	Enum(nil),
 	Format(""),
 	Type(""),
-	Nullable(false),
 )
 
 // FieldOnlyMarkers list field-specific validation markers (i.e. those markers that don't make
@@ -67,6 +66,7 @@ var FieldOnlyMarkers = []*markers.Definition{
 	markers.Must(markers.MakeDefinition("kubebuilder:validation:Required", markers.DescribesField, struct{}{})),
 	markers.Must(markers.MakeDefinition("kubebuilder:validation:Optional", markers.DescribesField, struct{}{})),
 	markers.Must(markers.MakeDefinition("optional", markers.DescribesField, struct{}{})),
+	markers.Must(markers.MakeDefinition("nullable", markers.DescribesField, Nullable{})),
 }
 
 func init() {
@@ -77,6 +77,8 @@ func init() {
 		typDef.Target = markers.DescribesType
 		AllDefinitions = append(AllDefinitions, &typDef)
 	}
+
+	AllDefinitions = append(AllDefinitions, FieldOnlyMarkers...)
 }
 
 type Maximum int
@@ -96,7 +98,7 @@ type UniqueItems bool
 type Enum []interface{}
 type Format string
 type Type string
-type Nullable bool
+type Nullable struct{}
 type Required struct{}
 type Optional struct{}
 
@@ -222,6 +224,6 @@ func (m Type) ApplyToSchema(schema *v1beta1.JSONSchemaProps) error {
 func (m Type) ApplyFirst() {}
 
 func (m Nullable) ApplyToSchema(schema *v1beta1.JSONSchemaProps) error {
-	schema.Nullable = bool(m)
+	schema.Nullable = true
 	return nil
 }


Test patch:
diff --git a/pkg/crd/parser_integration_test.go b/pkg/crd/parser_integration_test.go
index 97e11301..57389286 100644
--- a/pkg/crd/parser_integration_test.go
+++ b/pkg/crd/parser_integration_test.go
@@ -80,6 +80,6 @@ var _ = Describe("CRD Generation From Parsing to CustomResourceDefinition", func
 		Expect(yaml.Unmarshal(expectedFile, &crd)).To(Succeed())
 
 		By("comparing the two")
-		Expect(parser.CustomResourceDefinitions[groupKind]).To(Equal(crd), "%s", cmp.Diff(parser.CustomResourceDefinitions[groupKind], crd))
+		Expect(parser.CustomResourceDefinitions[groupKind]).To(Equal(crd), "type not as expected, check pkg/crd/testdata/README.md for more details.\n\nDiff:\n\n%s", cmp.Diff(parser.CustomResourceDefinitions[groupKind], crd))
 	})
 })
diff --git a/pkg/crd/testdata/README.md b/pkg/crd/testdata/README.md
new file mode 100644
index 00000000..4192e808
--- /dev/null
+++ b/pkg/crd/testdata/README.md
@@ -0,0 +1,23 @@
+# CRD Integration Test testdata
+
+This contains a tiny module used for testdata for the CRD integration
+test.  The directory should always be called testdata, so Go treats it
+specially.
+
+The `cronjob_types.go` file contains the input types, and is loosely based
+on the CronJob tutorial from the [KubeBuilder
+Book](https://book.kubebuilder.io/cronjob-tutorial.html), but with added
+fields to test additional markers and generation behavior.
+
+If you add a new marker, re-generate the golden output file,
+`testdata.kubebuilder.io_cronjobs.yaml`, with:
+
+```bash
+$ /path/to/current/build/of/controller-gen crd paths=. output:dir=.
+```
+
+Make sure you review the diff to ensure that it only contains the desired
+changes!
+
+If you didn't add a new marker and this output changes, make sure you have
+a good explanation for why generated output needs to change!
diff --git a/pkg/crd/testdata/cronjob_types.go b/pkg/crd/testdata/cronjob_types.go
index 008fc6a7..208b3cdd 100644
--- a/pkg/crd/testdata/cronjob_types.go
+++ b/pkg/crd/testdata/cronjob_types.go
@@ -62,6 +62,10 @@ type CronJobSpec struct {
 	// This tests byte slice schema generation.
 	BinaryName []byte `json:"binaryName"`
 
+	// This tests that nullable works correctly
+	// +nullable
+	CanBeNull string `json:"canBeNull"`
+
 	// Specifies the job that will be created when executing a CronJob.
 	JobTemplate batchv1beta1.JobTemplateSpec `json:"jobTemplate"`
 
diff --git a/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml b/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml
index cd9ea5a5..dbf4bd03 100644
--- a/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml
+++ b/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml
@@ -427,6 +427,10 @@ spec:
                 description: This tests byte slice schema generation.
                 format: byte
                 type: string
+              canBeNull:
+                description: This tests that nullable works correctly
+                nullable: true
+                type: string
               concurrencyPolicy:
                 description: 'Specifies how to treat concurrent executions of a Job.
                   Valid values are: - "Allow" (default): allows CronJobs to run concurrently;
@@ -6197,6 +6201,7 @@ spec:
                 type: boolean
             required:
             - binaryName
+            - canBeNull
             - jobTemplate
             - schedule
             type: object