⚠️ Convert ServerMetadata from a map to a list #1828
Conversation
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mdbooth 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 |
k8s-ci-robot
added
needs-rebase
mdbooth
force-pushed
the
server-metadata
branch
from
2f85d39 to
f19e3df
Compare
k8s-ci-robot
removed
the
needs-rebase
| type: array | ||
| x-kubernetes-list-map-keys: | ||
| - key | ||
| x-kubernetes-list-type: map |
There was a problem hiding this comment.
Will this have more consequences, like e.g. not allowing some characters because K8s doesn't allow them, but OpenStack does?
There was a problem hiding this comment.
I don't think there are any constraints on map keys. The documentation only says:
The key fields must be scalars.
They don't have the same constraints as names.
| // Value is the server metadata value | ||
| // kubebuilder:validation:MaxLength:=255 | ||
| Value string `json:"value"` | ||
| } |
There was a problem hiding this comment.
I think we may have other K/V settings , maybe something like Metadata then others reuse it?
There was a problem hiding this comment.
This lead me down an interesting rabbit hole which lead here: https://kubernetes.slack.com/archives/C05G4NJ6P6X/p1705486348670669
cc @lentzi90
I don't think so. The only other similar use I found was ValueSpecs, and the above rabbit hole lead me to believe we might want to remove that.
This struct has a 255 character limit on keys and values, which would likely not apply elsewhere.
If we did find another user for it, we could always DRY it later, as struct names don't form part of the API.
For now I think this is ok.
There was a problem hiding this comment.
ok, just check in case as I think metadata KV is widely used , but I agree 255 might be a thing not widely used
|
/hold for docs |
k8s-ci-robot
added
the
do-not-merge/hold
EmilienM
force-pushed
the
server-metadata
branch
from
f19e3df to
9cf43ed
Compare
|
/hold cancel I'll ask another reviewer to check it before lgtm. |
k8s-ci-robot
removed
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
EmilienM
force-pushed
the
server-metadata
branch
from
9cf43ed to
980b91b
Compare
k8s-ci-robot
removed
the
lgtm
|
It's failing on conversion now, Matt changed something with fuzzer, it'll need adjustment. I'll have a look tomorrow. |
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
While we're changing it, we also add validation that keys and values don't exceed 255 characters.
mdbooth
force-pushed
the
server-metadata
branch
from
980b91b to
dfb963d
Compare
|
/lgtm |
k8s-ci-robot
removed
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
Thanks Matt for fixing conversion. |
While we're changing it, we also add validation that keys and values don't exceed 255 characters.
Fixes: #1684
TODO:
/hold