Add structType support #2
Conversation
|
( Putting WIP for now because I see test failures due to this validation: Test failures show that |
It maps to x-kubernetes-map-type in the openapi schema, similar to the mapType marker.
| @@ -29,6 +29,7 @@ var TopologyMarkers = []*definitionWithHelp{ | |||
| must(markers.MakeDefinition("listType", markers.DescribesField, ListType(""))), | |||
| must(markers.MakeDefinition("listMapKey", markers.DescribesField, ListMapKey(""))), | |||
| must(markers.MakeDefinition("mapType", markers.DescribesField, MapType(""))), | |||
| must(markers.MakeDefinition("structType", markers.DescribesField, StructType(""))), | |||
There was a problem hiding this comment.
listType and listMapKey are not ordered :-/ Not your fault ...
There was a problem hiding this comment.
listType and listMapKey are not ordered
Shall I change that as part of this PR? It's a tiny bit of scope creep, but especially if we'll squash commits as part of kubernetes-sigs#347, it's probably not a huge deal.
There was a problem hiding this comment.
Yeah, I'll re-order when I rebase/squash! :-)
pkg/crd/markers/topology.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (s StructType) ApplyFirst() {} |
There was a problem hiding this comment.
Mainly driven by this: https://github.com/kubernetes-sigs/controller-tools/blob/4752ed2de7d2fc1b6b18398bf26cf2ce6b53cd94/pkg/crd/markers/doc.go#L27-L29. If I got it right, it suggests that "top-level" types (e.g. lists, as opposed to list items) can implement this method to ensure that their schemas will be applied before types that don't implement it.
The comment also says it's discouraged and a hack so if all this is not quite right, I'm happy to remove it. No tests fail if I remove ApplyFirst for StructType, for what it's worth 🤔
There was a problem hiding this comment.
I would only use it if there is an actual dependency between two types (like we do for ListType). But I don't think that's the case here.
pkg/crd/markers/topology.go
Outdated
| func (m MapType) ApplyFirst() {} | ||
|
|
||
| func (s StructType) ApplyToSchema(schema *apiext.JSONSchemaProps) error { | ||
| if schema.Type != "object" { |
There was a problem hiding this comment.
I suspect "object" is the default and is thus not required.
There was a problem hiding this comment.
Aha, makes sense. I went for "object" || "" just to be on the strict side. Let me know what you think, happy to remove the check completely if we think it doesn't add much.
|
One minor nit, but looks good otherwise. |
|
Thanks! |
Similarly to #1, this PR adds a
+structTypemarker, that maps tox-kubernetes-map-typein kube-openapi.cc @apelisse