Repository: kubernetes-sigs/controller-tools
Pull request #617 (2021-09-16 22:13:46)

✨ Allow maps of arrays of non-string types
Lifts the restriction currently imposed on map value types (that if they are arrays they must be arrays of `string`s), per https://github.com/kubernetes-sigs/controller-tools/pull/518#issuecomment-920387673. We have encountered this while trying to use `controller-gen` (https://github.com/Azure/azure-service-operator/issues/1489).

Also add a way to add simple schema-transform tests, rather than by adding items to `cronjob_types.go` (where the output is validated by inspection). 

k8s-ci-robot: Hi @Porges. Thanks for your PR.

I'm waiting for a [kubernetes-sigs](https://github.com/orgs/kubernetes-sigs/people) member to verify that this patch is reasonable to test. If it is, they should reply with `/ok-to-test` on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should [join the org](https://git.k8s.io/community/community-membership.md#member) to skip this step.

Once the patch is verified, the new status will be reflected by the `ok-to-test` label.

I understand the commands that are listed [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fcontroller-tools).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

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/617#" title="Author self-approved">Porges</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **pwittrock** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @pwittrock` 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":["pwittrock"]} -->
Porges: @alvaroaleman please take a look, this is a follow-on to the [last PR](https://github.com/kubernetes-sigs/controller-tools/pull/518) 
alvaroaleman: /lgtm
k8s-ci-robot: @Porges: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-controller-tools-test-master | 5f23bd709bdf16ce0f7764a48e3019927d37cfab | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_controller-tools/617/pull-controller-tools-test-master/1438898141603565568) | `/test pull-controller-tools-test-master`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=controller-tools&pr=617). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3APorges). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/controller-tools/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
alvaroaleman: /lgtm cancel
floats are generally not allowed, ref https://github.com/kubernetes-sigs/controller-tools/issues/245
Porges: > @alvaroaleman  floats are generally not allowed, ref #245

Corrected golden file test.

alvaroaleman: /ok-to-test
k8s-ci-robot: @Porges: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-controller-tools-test-master | e1a7c2d74345f90c5ec59c8c7b6ed8b113cc1a20 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_controller-tools/617/pull-controller-tools-test-master/1439706235623444480) | `/test pull-controller-tools-test-master`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=controller-tools&pr=617). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3APorges). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/controller-tools/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
alvaroaleman: /label tide/merge-method-squash
@porges Can you fix the compile error please?
Porges: @alvaroaleman looks like a bad linter, have updated code to fix.
alvaroaleman: /lgtm

Patch:
diff --git a/pkg/crd/schema.go b/pkg/crd/schema.go
index be6103a0..b665817f 100644
--- a/pkg/crd/schema.go
+++ b/pkg/crd/schema.go
@@ -39,12 +39,10 @@ const (
 	defPrefix = "#/definitions/"
 )
 
-var (
-	// byteType is the types.Type for byte (see the types documention
-	// for why we need to look this up in the Universe), saved
-	// for quick comparison.
-	byteType = types.Universe.Lookup("byte").Type()
-)
+// byteType is the types.Type for byte (see the types documention
+// for why we need to look this up in the Universe), saved
+// for quick comparison.
+var byteType = types.Universe.Lookup("byte").Type()
 
 // SchemaMarker is any marker that needs to modify the schema of the underlying type or field.
 type SchemaMarker interface {
@@ -309,10 +307,6 @@ func mapToSchema(ctx *schemaContext, mapType *ast.MapType) *apiext.JSONSchemaPro
 		valSchema = namedToSchema(ctx.ForInfo(&markers.TypeInfo{}), val)
 	case *ast.ArrayType:
 		valSchema = arrayToSchema(ctx.ForInfo(&markers.TypeInfo{}), val)
-		if valSchema.Type == "array" && valSchema.Items.Schema.Type != "string" {
-			ctx.pkg.AddError(loader.ErrFromNode(fmt.Errorf("not a supported map value type: %T", mapType.Value), mapType.Value))
-			return &apiext.JSONSchemaProps{}
-		}
 	case *ast.StarExpr:
 		valSchema = typeToSchema(ctx.ForInfo(&markers.TypeInfo{}), val)
 	case *ast.MapType:


Test patch:
diff --git a/pkg/crd/schema_test.go b/pkg/crd/schema_test.go
new file mode 100644
index 00000000..bd46858e
--- /dev/null
+++ b/pkg/crd/schema_test.go
@@ -0,0 +1,112 @@
+/*
+Copyright 2019 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package crd
+
+import (
+	"go/ast"
+	"strings"
+	"testing"
+
+	"github.com/onsi/gomega"
+	"golang.org/x/tools/go/packages"
+	pkgstest "golang.org/x/tools/go/packages/packagestest"
+	apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	testloader "sigs.k8s.io/controller-tools/pkg/loader/testutils"
+	"sigs.k8s.io/controller-tools/pkg/markers"
+)
+
+func transform(t *testing.T, expr string) *apiext.JSONSchemaProps {
+	// this is *very* hacky but I haven’t found a simple way
+	// to get an ast.Expr with all the associated metadata required
+	// to run typeToSchema upon it:
+
+	moduleName := "sigs.k8s.io/controller-tools/pkg/crd"
+	modules := []pkgstest.Module{
+		{
+			Name: moduleName,
+			Files: map[string]interface{}{
+				"test.go": `
+				package crd 
+				type Test ` + expr,
+			},
+		},
+	}
+
+	pkgs, exported, err := testloader.LoadFakeRoots(pkgstest.Modules, modules, moduleName)
+	if exported != nil {
+		t.Cleanup(exported.Cleanup)
+	}
+
+	if err != nil {
+		t.Fatalf("unable to load fake package: %s", err)
+	}
+
+	if len(pkgs) != 1 {
+		t.Fatal("expected to parse only one package")
+	}
+
+	pkg := pkgs[0]
+	pkg.NeedTypesInfo()
+	failIfErrors(t, pkg.Errors)
+
+	schemaContext := newSchemaContext(pkg, nil, true).ForInfo(&markers.TypeInfo{})
+	// yick: grab the only type definition
+	definedType := pkg.Syntax[0].Decls[0].(*ast.GenDecl).Specs[0].(*ast.TypeSpec).Type
+	result := typeToSchema(schemaContext, definedType)
+	failIfErrors(t, pkg.Errors)
+	return result
+}
+
+func failIfErrors(t *testing.T, errs []packages.Error) {
+	if len(errs) > 0 {
+		var msgs []string
+		for _, e := range errs {
+			msgs = append(msgs, e.Msg)
+		}
+
+		t.Fatalf("error loading fake package: %s", strings.Join(msgs, "; "))
+	}
+}
+
+var arrayOfNumbersSchema *apiext.JSONSchemaProps = &apiext.JSONSchemaProps{
+	Type: "array",
+	Items: &apiext.JSONSchemaPropsOrArray{
+		Schema: &apiext.JSONSchemaProps{
+			Type: "number",
+		},
+	},
+}
+
+func Test_Schema_ArrayOfFloat32(t *testing.T) {
+	g := gomega.NewWithT(t)
+
+	output := transform(t, "[]float32")
+	g.Expect(output).To(gomega.Equal(arrayOfNumbersSchema))
+}
+
+func Test_Schema_MapOfStringToArrayOfFloat32(t *testing.T) {
+	g := gomega.NewWithT(t)
+
+	output := transform(t, "map[string][]float32")
+	g.Expect(output).To(gomega.Equal(&apiext.JSONSchemaProps{
+		Type: "object",
+		AdditionalProperties: &apiext.JSONSchemaPropsOrBool{
+			Allows: true,
+			Schema: arrayOfNumbersSchema,
+		},
+	}))
+}
diff --git a/pkg/crd/testdata/cronjob_types.go b/pkg/crd/testdata/cronjob_types.go
index 462cdd84..f7aac8cb 100644
--- a/pkg/crd/testdata/cronjob_types.go
+++ b/pkg/crd/testdata/cronjob_types.go
@@ -178,6 +178,9 @@ type CronJobSpec struct {
 
 	// Checks that maps containing types that contain maps work
 	ContainsNestedMapMap map[string]ContainsNestedMap `json:"nestedMapInStruct,omitempty"`
+
+	// Maps of arrays of things-that-aren’t-strings are permitted
+	MapOfArraysOfFloats map[string][]bool `json:"mapOfArraysOfFloats,omitempty"`
 }
 
 type ContainsNestedMap struct {
diff --git a/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml b/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml
index 845ffceb..ceb55772 100644
--- a/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml
+++ b/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml
@@ -7176,6 +7176,13 @@ spec:
                 - bar
                 - foo
                 type: object
+              mapOfArraysOfFloats:
+                additionalProperties:
+                  items:
+                    type: boolean
+                  type: array
+                description: Maps of arrays of things-that-aren’t-strings are permitted
+                type: object
               mapOfInfo:
                 additionalProperties:
                   format: byte