nfd-master: fix panic on NodeFeature delete#2478
nfd-master: fix panic on NodeFeature delete#2478k8s-ci-robot merged 1 commit intokubernetes-sigs:masterfrom
Conversation
✅ Deploy Preview for kubernetes-sigs-nfd ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
|
|
Welcome @samsar! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @samsar. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
k8s-ci-robot
added
cncf-cla: no
| klog.V(2).InfoS("found stale NodeFeatureGroup object", "object", obj) | ||
| obj = deletedFinalStateUnknown.Obj | ||
| } | ||
| nfg := obj.(*nfdv1alpha1.NodeFeatureGroup) |
There was a problem hiding this comment.
Per your judgment: would something like pkg/nfd-gc/nfd-gc.go:131-134 be extra here (or that one alone would do).
There was a problem hiding this comment.
Good point. I think this handler will always be called with the right object type, but I added the type assertion to both handlers just to be extra safe
fmuyassarov
left a comment
fmuyassarov
left a comment
There was a problem hiding this comment.
I think this a good defensive delete handler pattern and I don't see an issue with it.
WDYT @marquiz
|
One thing I would ask to do though is to squash the commits. |
samsar
force-pushed
the
fix-2470-nfd-master-panic
branch
2 times, most recently
from
2d5dcf8 to
c2178d5
Compare
Consolidate the panic fix and follow-up cleanup into a single change set with safer type assertions and clearer error logging.
samsar
force-pushed
the
fix-2470-nfd-master-panic
branch
from
c2178d5 to
72658da
Compare
Done! |
k8s-ci-robot
added
ok-to-test
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: marquiz, ozhuraki, samsar The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Yes 👍 |
|
/cherry-pick release-0.18 |
|
@marquiz: once the present PR merges, I will cherry-pick it on top of DetailsIn 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-sigs/prow repository. |
fmuyassarov
left a comment
fmuyassarov
left a comment
There was a problem hiding this comment.
/retitle nfd-master: fix panic on NodeFeature delete
/lgtm
k8s-ci-robot
changed the title
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. DetailsGit tree hash: a3bb4876bf98002ed4c9e7e1b708c0505c58db56 |
|
@marquiz: new pull request created: #2487 DetailsIn 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-sigs/prow repository. |
6 participants
Fixes #2470