Separate OpenAPI V2 and V3 config in V3 builder #431
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Jefftree 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
the
approved
Jefftree
changed the title
k8s-ci-robot
added
do-not-merge/work-in-progress
Jefftree
force-pushed
the
separate-v3-config
branch
from
e934b2f to
406ddbd
Compare
k8s-ci-robot
added
size/M
Jefftree
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
Jefftree
changed the title
|
/assign @apelisse |
|
k/k PR is a bit more involved: kubernetes/kubernetes#121010 |
| @@ -315,12 +315,12 @@ func (o *openAPI) buildOpenAPISpec(webServices []common.RouteContainer) error { | |||
| // BuildOpenAPISpec builds OpenAPI v3 spec given a list of route containers and common.Config to customize it. | |||
| // | |||
| // Deprecated: BuildOpenAPISpecFromRoutes should be used instead. | |||
| func BuildOpenAPISpec(webServices []*restful.WebService, config *common.Config) (*spec3.OpenAPI, error) { | |||
| func BuildOpenAPISpec(webServices []*restful.WebService, config *common.OpenAPIV3Config) (*spec3.OpenAPI, error) { | |||
There was a problem hiding this comment.
how breaking is this? We have to wonder how much we break now when we make these changes. I'm assuming that only the apiserver is using that code though.
There was a problem hiding this comment.
cc @ryantxu since this will affect your workflow.
My understanding is that only apiserver uses this code, and the v3 path is less used than v2. It's also technically "wrong" to use the v2 config for v3 and I don't want to encourage people going down/sticking to that path. Things like PostProcessSpec(*spec.Swagger) ... (https://github.com/kubernetes/kube-openapi/blob/master/pkg/common/common.go#L114-L115) that takes a v2 spec is ignored when building v3.
I'm happy to create a new function if we want though.
There was a problem hiding this comment.
FWIW, the standard codegen tools do not produce code that calls this directly. The breaking change will be in kubernetes/kubernetes#121010 😬
But honestly it feels more like a bug fix since the v2 options do not cleanly apply to v3
| o := openAPI{ | ||
| config: common.ConvertConfigToV3(config), |
There was a problem hiding this comment.
Should we consider removing this function?
|
Yeah, I think the breaking change is fine here. LGTM, just one question. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
Added some klog fixes as well, as they seemed to be needed. The replace k8s.io/kube-openapi => k8s.io/kube-openapi v0.0.0-20230928195430-ce36a0c3bb67 statement is added to work around an issue caused by kubernetes/kube-openapi#431 Signed-off-by: Jan Fajerski <[email protected]>
Added some klog fixes as well, as they seemed to be needed. The replace k8s.io/kube-openapi => k8s.io/kube-openapi v0.0.0-20230928195430-ce36a0c3bb67 statement is added to work around an issue caused by kubernetes/kube-openapi#431 Signed-off-by: Jan Fajerski <[email protected]>
Added some klog fixes as well, as they seemed to be needed. The replace k8s.io/kube-openapi => k8s.io/kube-openapi v0.0.0-20230928195430-ce36a0c3bb67 statement is added to work around an issue caused by kubernetes/kube-openapi#431 Signed-off-by: Jan Fajerski <[email protected]>
Added some klog fixes as well, as they seemed to be needed. The replace k8s.io/kube-openapi => k8s.io/kube-openapi v0.0.0-20230928195430-ce36a0c3bb67 statement is added to work around an issue caused by kubernetes/kube-openapi#431 Signed-off-by: Jan Fajerski <[email protected]>
Decouple v2 and v3 configs to support #420 properly