-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
make apiservices.apiregistration.k8s.io discoverable in openapi/v3 #118879
make apiservices.apiregistration.k8s.io discoverable in openapi/v3 #118879
Conversation
side topic: do we want |
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.
Thanks for bringing this up and fixing it. A couple small comments otherwise LGTM
@@ -239,3 +290,20 @@ func sendReq(t *testing.T, handler http.Handler, path string) []byte { | |||
handler.ServeHTTP(writer, req) | |||
return writer.data | |||
} | |||
|
|||
func testGetOpenAPIDefinitions(_ openapicommon.ReferenceCallback) map[string]openapicommon.OpenAPIDefinition { |
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: Rename to something to indicate that this is returning and APIService definition? Same for buildTestOpenAPIDefinition().
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.
using APIService prefix now
} | ||
aggregatorWebServicesHandler.UpdateGroupVersion(gv, spec) | ||
} | ||
apiServiceName := fmt.Sprintf(localDelegateChainNamePattern, i) |
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.
This isn't really a delegation handler in the traditional sense. The aggregator serves the path, can we use a constant name for this to not affect the local delegation numbers?
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.
renamed to k8s_internal_local_kube_aggregator_types
to at least have the same prefix as the delegation handlers
Actually this is a duplicate of #118884 |
wow @atiratree I was reviewing your other PR for other resources that fail with |
@alexzielenski ow, what a coincidence. Sorry about that. Will take a look at your PR. |
/triage accepted |
2ef4dd8
to
b4aa9fa
Compare
@Jefftree updated the PR, using @alexzielenski simpler implementation now |
b4aa9fa
to
cad3388
Compare
/triage accepted |
/lgtm |
LGTM label has been added. Git tree hash: a793d00d337a250799f0f9fd6dca21cd35b4f8ca
|
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: atiratree, deads2k 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 |
I have opened a cherry-pick into 1.27 #119841. Not sure if we should backport this further back, but we should at least do 1.27, since the OpenAPIV3 feature has been graduated to stable. |
…18879-upstream-release-1.27 Automated cherry pick of #118879: expose apiregistration.k8s.io into OpenAPIV3
was not being published until mid 1.27 release cycle kubernetes/kubernetes#118879
was not being published until mid 1.27 release cycle kubernetes/kubernetes#118879
What type of PR is this?
/kind bug
/kind regression
/sig api-machinery
What this PR does / why we need it:
make apiservices.apiregistration.k8s.io discoverable in openapi/v3
Which issue(s) this PR fixes:
Fixes #118878
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: