use separate scheme to serve the kube-aggregator #42672
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED The following people have approved this PR: deads2k Needs approval from an approver in each of these OWNERS Files: We suggest the following people: |
k8s-github-robot
added
approved
|
This change is |
deads2k
force-pushed
the
agg-24-use-custom-scheme
branch
from
ab87ef7 to
21e6999
Compare
k8s-github-robot
removed
the
approved
ncdc
added
release-note-none
| ) | ||
|
|
||
| func init() { |
There was a problem hiding this comment.
Is there a way to do this in a normal func instead of init?
There was a problem hiding this comment.
Is there a way to do this in a normal func instead of init?
Maybe. We've always initialized a scheme on init, you want to change it to run through an accessor calling a func using a global sync.Once? I can if you feel strongly.
There was a problem hiding this comment.
It's not like you're differing from current patterns. Long-term, I'd like to get away from init but it's fine for now.
| func init() { | ||
| install.Install(groupFactoryRegistry, registry, Scheme) | ||
|
|
||
| // we need to add the options to empty v1 |
There was a problem hiding this comment.
what options?
Adds options objects to the scheme.
There was a problem hiding this comment.
Like ListOptions?
yes
| install.Install(groupFactoryRegistry, registry, Scheme) | ||
|
|
||
| // we need to add the options to empty v1 | ||
| metav1.AddToGroupVersion(Scheme, schema.GroupVersion{Version: "v1"}) |
There was a problem hiding this comment.
Or use unversioned from the next line
There was a problem hiding this comment.
Specify explicit Group: ""
ok
There was a problem hiding this comment.
Or use unversioned from the next line
these are logically distinct, so separation makes sense.
There was a problem hiding this comment.
But they are identical, right?
For now
There was a problem hiding this comment.
Done and clarified the comment.
|
Only minor comments. Do we have any tests (maybe integration?) that can catch any possible regressions in the aggregator apiserver? |
Yes. We have an integration test that fires it up and makes sure it can create APIService resources. |
deads2k
force-pushed
the
agg-24-use-custom-scheme
branch
from
21e6999 to
1dc8ae9
Compare
deads2k
added
approved
|
@k8s-bot gce etcd3 e2e test this |
k8s-github-robot
added
the
needs-rebase
fejta
removed
the
needs-rebase
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue (batch tested with PRs 42672, 42770, 42818, 42820, 40849) |
|
@deads2k: The following test(s) failed:
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Automatic merge from submit-queue (batch tested with PRs 43694, 41262, 42911) combine kube-apiserver and kube-aggregator This combines several pulls currently in progress and wires them together. The aggregator sits in front of the normal kube-apiserver and allows local fallthrough instead of proxying. @kubernetes/sig-api-machinery-misc @DirectXMan12 since you seem invested, your life will get easier @luxas FYI since you've started trying to wire something together. Dependent Pulls LGTM: - [x] #42801 - [x] #42886 - [x] #42900 - [x] #42732 - [x] #42672 - [x] #43141 - [x] #43076 - [x] #43149 - [x] #43226 - [x] #43144
This removes a hack which used the client scheme to serve the kube-aggregator. This switches it to run from its own scheme.
@kubernetes/sig-api-machinery-pr-reviews
@ncdc