-
Notifications
You must be signed in to change notification settings - Fork 416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Add support for ListType, ListMapKeys, MapType and StructType #347
Conversation
Welcome @apelisse! |
853bb97
to
2fcb40c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few nits inline
pkg/crd/markers/topology.go
Outdated
} | ||
|
||
// +controllertools:marker:generateHelp:category="CRD topology" | ||
// ListType specifies the type of list. A list can be: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: split in their own paragraph
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
specifies data structure the list represents (map, set, atomic).
a list may be....
pkg/crd/markers/topology.go
Outdated
// +controllertools:marker:generateHelp:category="CRD topology" | ||
// ListType specifies the type of list. A list can be: | ||
// - a "map": it needs to have a key field, which will be used to build an | ||
// associative list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe give examples from k8s
pkg/crd/markers/topology.go
Outdated
// ToplogyMarkers list topology markers (i.e. markers that specify if a | ||
// list is an associative-list or a set, or if a map is atomic or not). | ||
var TopologyMarkers = []*definitionWithHelp{ | ||
must(markers.MakeDefinition("listType", markers.DescribesField, ListType(""))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
list:type
list:key
??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too late :-/
pkg/crd/markers/topology.go
Outdated
|
||
func (l ListType) ApplyToSchema(schema *v1beta1.JSONSchemaProps) error { | ||
if schema.Type != "array" { | ||
return fmt.Errorf("must apply uniqueitems to an array") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: uniqueitems?
2fcb40c
to
b9798c7
Compare
I think I fixed everything, thanks! |
I think it'd be great to implement mapType and structType on top of these two here. mapType and structType are two different "markers", but they should generate the same |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiny nit, but we might just want to do that in a follow-up
pkg/crd/markers/topology.go
Outdated
AllDefinitions = append(AllDefinitions, TopologyMarkers...) | ||
} | ||
|
||
// +controllertools:marker:generateHelp:category="CRD topology" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: #345 (comment)
(but let's not block on that)
b9798c7
to
a6df9d4
Compare
@mariantalla Do we miss |
e45eaa2
to
88dd335
Compare
Rebased and squashed some commits |
1cbd921
to
a93adca
Compare
rebased/squashed and fixed a few minor nits. @mariantalla can you make sure I didn't butcher your code? :-o |
/retest |
a93adca
to
bde00a7
Compare
bde00a7
to
776e34c
Compare
EDIT: tweaked docs to show up nicely in HTML form (list items need to be blank-line-separated or they'll be merged into the same line by the docs generator) EDIT: made sure that generated help was actually used for the added markers |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: apelisse, DirectXMan12 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:
Approvers can indicate their approval by writing |
These two new markers allow support for associative lists and sets.
This adds support for x-kubernetes-map-type, which controls how to merge maps in server-side apply.
It maps to x-kubernetes-map-type in the openapi schema, similar to the mapType marker.
776e34c
to
6694c6b
Compare
Fixed that typo that was making CI unhappy, can you re-apply lgtm @DirectXMan12? Thanks |
These two new markers allow support for associative lists and sets.
Fixes kubernetes-sigs/controller-runtime#638
This is clearly not done for many reasons:
I think we can move with just these new markers and work on other markers when they are available in k/k: kubernetes/kubernetes#84113