search

kubernetes / kube-openapi

Kubernetes OpenAPI spec generation & serving
Apache License 2.0
317 stars 205 forks source link

spec: support string slices in GetStringSlice #413

Closed alexzielenski closed 1 year ago

alexzielenski commented 1 year ago

makes GetStringSlice support slices of strings

ConvertJSONSchemaPropsWithPostProcess in k/k directly places its []string of x-kubernetes-list-map-keys into the extensions map, which are then used with validation code.

Running into this problem of GetStringSlice not seeing the MapKeys added by this function, since they are []string and it only supports []interface{}

Adding this logic alleviates the issue and makes the implementation match the expected behavior of the function

/cc @apelisse

apelisse commented 1 year ago

/approve /lgtm

k8s-ci-robot commented 1 year ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alexzielenski, apelisse Once this PR has been reviewed and has the lgtm label, please assign liggitt for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files: - **[pkg/validation/OWNERS](https://github.com/kubernetes/kube-openapi/blob/master/pkg/validation/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
sttts commented 1 year ago

Am not convinced of this. Everywhere in kube we have the convention that JSON unstructured data must be well-typed. []string is not a valid JSON type. Extending this function here IMO just hides errors. The right solution is to fix ConvertJSONSchemaPropsWithPostProcess. I wished Go was able express this invariant 🤷‍♂️

sttts commented 1 year ago

https://github.com/kubernetes/kubernetes/blob/2a91bd1dfdd2e293b9ec017ea3a976ecc2ecd545/staging/src/k8s.io/apimachinery/pkg/runtime/converter.go#L614 has the definition of what valid JSON values are in kube code.

alexzielenski commented 1 year ago

/close

in favor of changing ConvertJSONSchemaPropsWithPostProcess

k8s-ci-robot commented 1 year ago

@alexzielenski: Closed this PR.

In response to [this](https://github.com/kubernetes/kube-openapi/pull/413#issuecomment-1636397104): >/close > >in favor of changing ConvertJSONSchemaPropsWithPostProcess 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.