🐛 Don't generate description on metadata of CRD structural schema

[kevindelgado]

What

Ensures that generated CRDs do not have a description generated from godoc comments on the ObjectMeta of the corresponding go type.

Why

Fixes #386

CRD structural schema requires that if metadata is specified, then only restrictions on metadata.name and metadata.generateName are allowed.

metadata.description is not allowed

xref: kubernetes/kubernetes#86800

[kevindelgado]

/assign @DirectXMan12
/cc @tamalsaha

[k8s-ci-robot]

@kevindelgado: GitHub didn't allow me to request PR reviews from the following users: tamalsaha.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/assign @DirectXMan12
/cc @tamalsaha

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[kevindelgado]

Was waiting for #499 to land with the basic gen_integration_test before nudging this, but that's merged now, can you take a look @DirectXMan12

[kevindelgado]

friendly ping @DirectXMan12

[DirectXMan12]

/lgtm
/approve

[DirectXMan12]

/hold

[DirectXMan12]

/lgtm cancel
/approve cancel

we should be doing this in the output openapi, not in the input godoc

[kevindelgado]

metadata description is now stripped on the output open API schema instead of the input godoc.

It definitely doesn't feel as clean as the one line fix on the input, but I get that we might want to hang onto the raw godoc for as long as possible.

Let me know if you have any suggestions to clean this up. PTAL @DirectXMan12

[kevindelgado]

Is this warning actually useful or can we assume that users know their ObjectMeta godoc comments will not make it into the CRD schema?

[whalecold]

Hi, I test your code in myself environment but it don't work well because of the other fields may container the struct JSONSchemaPropsalso. I try to changed the code as follow and it works. That's only a suggestion.

	if v == nil {
		return
	}
	for str, property := range v.Properties {
		if str == "metadata" {
			if property.Description != "" {
				fmt.Fprintf(os.Stderr, "Warning: version metadata description unsupported. Removing description: %s\n", property.Description)
				property.Description = ""
			}
		}
		if property.Items != nil {
			removeDescriptionFromMetadataProps(property.Items.Schema)
			for i := range property.Items.JSONSchemas {
				removeDescriptionFromMetadataProps(&property.Items.JSONSchemas[i])
			}
		}
		if property.AdditionalProperties != nil {
			removeDescriptionFromMetadataProps(property.AdditionalProperties.Schema)
		}
		if property.AdditionalItems != nil {
			removeDescriptionFromMetadataProps(property.AdditionalItems.Schema)
		}
		removeDescriptionFromMetadataProps(&property)
		v.Properties[str] = property
	}

[whalecold]

friendly ping @kevindelgado

[kevindelgado]

Thanks!

[DirectXMan12]

top-level metadata shouldn't have any fields -- it's explicitly stripped because the kubernetes API server replaces/ignores most of it.

AFAIK, the API server only enforces this constraint on the top-level metadata field, not any embedded metadata, so it should be sufficient to simply clear description from the the root metadata field, and leave it at that.

@whalecold if this doesn't work, can you please provide an example that's giving you issues

[DirectXMan12]

I think this warning might actually just confuse people too, so prob best to remove it -- it's gonna hit a lot of codebases and doesn't really have a material impact on them.

[kevindelgado]

Not loving the duplicated functions for v1 and v1beta1, but I couldn't find a cleaner way to make it work. I thought of passing the generic runtime.Object into a single removeDescriptionFromMetadata() function, but it didn't seem any better since we already have to do the crdVersions check for removing defaults. Let me know if there's a better way.

[kevindelgado]

PTAL @DirectXMan12

[DirectXMan12]

why are we recursing here?

[kevindelgado]

See feedback from @whalecold here

I took it to mean that there could be objects embedded within the object (e.g. within the spec field), is that wrong? See the first commit for the non-recursive approach.

@whalecold can you provide an example of where this breaks if you don't recurse the fields that contain addition JSONSchemaProps?

[kevindelgado]

Reverted back to modifying only the top level metadata description, as @DirectXMan12 and I tested that kube apiserver accepts embedded objects with metadata descriptions.

[DirectXMan12]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, kevindelgado

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [DirectXMan12]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[DirectXMan12]

/hold cancel