✨ Implement Catalog for Runtime SDK #6393
Conversation
k8s-ci-robot
added
kind/feature
ykakarap
force-pushed
the
runtime-sdk-catalog
branch
4 times, most recently
from
1e6e292
to
66bd9fd
Compare
ykakarap
changed the title
internal/runtime/catalog/catalog.go
Outdated
| panic("Group must not be empty") | ||
| } | ||
| if gv.Version == "" { | ||
| panic("Version must not be empty") |
There was a problem hiding this comment.
Errors should start with a lower case letter.
There was a problem hiding this comment.
Let's keep it as is now, but just fyi. Those are not errors. Those are strings which are handed over to panics. I used upper case because the upstream scheme package was using uppercase consistently.
The reason why errors are lower case is because they are concatenated like this <error-message>: <error-message>: <error-message>. Uppercase would look awkward there.
This does not apply to panics.
ykakarap
force-pushed
the
runtime-sdk-catalog
branch
from
66bd9fd
to
5775f95
Compare
|
|
||
| // Singleton signals if the hook can only be implemented once on a | ||
| // Runtime Extension, e.g. like the DiscoveryHook. | ||
| Singleton bool |
There was a problem hiding this comment.
Q: do we really need this given that the DiscoveryHook is already treated as a special case during the registration process?
There was a problem hiding this comment.
This field becomes important when we are going to do OpenAPI spec generation. This can be used to distinguish between endpoints that are name specific vs endpoints that are not (singletons).
Example:
The Before Cluster Create hook is not a singleton and the end point will be name specific. Endpoint: /hooks.runtime.cluster.x-k8s.io/v1alpha1/beforeclusterupgrade/{name}.
The discovery endpoint is a singleton and the endpoint will be something like: /hooks.runtime.cluster.x-k8s.io/v1alpha1/discovery
There was a problem hiding this comment.
kk,
I'm not super happy to expose a knob for a what is a single, well-known special case now, but we can eventually re-iterate later (it could be also a simple rename to something that makes it more clear what are the implications of a singleton in this context).
Feel free to resolve the comment
There was a problem hiding this comment.
I would also try to avoid it, but I wasn't sure how. I didn't want to hard-code if hook == "Discovery" but maybe we should just do that. Let's reconsider on the runtime openapi gen PR.
ykakarap
force-pushed
the
runtime-sdk-catalog
branch
from
5775f95
to
1707cc3
Compare
Co-authored-by: sbueringer <sburingerst@vmware.com>
ykakarap
force-pushed
the
runtime-sdk-catalog
branch
from
1707cc3
to
52669e4
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
I'll take another closer look first thing tomorrow. But I don't expect major findings. |
|
Added 2 comments to the conversations above (informational for future PRs), but let's move on :) /lgtm |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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
What this PR does / why we need it:
Implement the Catalog useful for the Runtime SDK implementation.
Catalog is in many ways similar to schema but it extends to become the store for Runtime Hooks.
Catalog will be used to store the hooks, their mapping to the request and response objects, the open api generators, util functions, etc.
Part of #6330
/kind feature
/area runtime-sdk
@sbueringer @fabriziopandini @vincepri @CecileRobertMichon @enxebre