minor adjustments in the sample apiserver around resource creation. #48711
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @p0lyn0mial. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. 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. |
k8s-ci-robot
added
the
needs-ok-to-test
k8s-github-robot
added
size/M
|
@deads2k hopefully the changes will make you happy :) |
| v1alpha1storage["flunders"] = flunderstorage.RESTInPeace(Scheme, c.GenericConfig.RESTOptionsGetter) | ||
| v1alpha1storage["fischers"] = fischerstorage.RESTInPeace(Scheme, c.GenericConfig.RESTOptionsGetter) | ||
| v1alpha1storage["flunders"] = wardleregistry.RESTInPeace(flunderstorage.NewREST(Scheme, c.GenericConfig.RESTOptionsGetter)) | ||
| v1alpha1storage["fischers"] = wardleregistry.RESTInPeace(fischerstorage.NewREST(Scheme, c.GenericConfig.RESTOptionsGetter)) |
There was a problem hiding this comment.
at first glace, this is moving the constructor to a package I didn't expect
There was a problem hiding this comment.
if it helps we could change the pkg name to wardlestorage. Notice that RESTInPeace is just a wrapper. To give you more context #48444 (comment)
There was a problem hiding this comment.
at first glace, this is moving the constructor to a package I didn't expect
It's a wrapper we're trying out here to allow errors from REST construction without panics. We did a similar thing at a lower level in openshift and I wasn't a huge fan of where we put it.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
k8s-github-robot
added
the
approved
|
I think that the tests were very unstable at that time. I will rebase against the master. |
p0lyn0mial
force-pushed
the
sample_apiserver_small_improvements
branch
from
62322f0 to
6a3191a
Compare
k8s-github-robot
removed
the
lgtm
|
/retest |
|
@p0lyn0mial: you can't request testing unless you are a kubernetes member. In response to this:
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. |
|
@deads2k the bot is not obedient :( please, retest and retag. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, p0lyn0mial Associated issue requirement bypassed by: deads2k The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
@eparis how do we get @p0lyn0mial added to the whitelist to be tested? |
|
Automatic merge from submit-queue (batch tested with PRs 48997, 48595, 48898, 48711, 48972) |
What this PR does / why we need it: This PR improves
NewRESTmethod to support an error and nor-error path. The method is then wrapped in a convenient function which panics on error.Release note: