BugFix: Update ingress key function used for GC #881
Conversation
|
Hi @skmatti. 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 Once the patch is verified, the new status will be reflected by the 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. |
k8s-ci-robot
added
needs-ok-to-test
|
/assign mrhohn |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
pkg/utils/namer/utils.go
Outdated
| ) | ||
|
|
||
| // IngressKeyFunc returns ingress key used in Ingress Cache Store. | ||
| func IngressKeyFunc(ing *v1beta1.Ingress) string { |
There was a problem hiding this comment.
Seems like no one is calling this function anymore?
There was a problem hiding this comment.
This line is not modified in this change. So does not show this in diff.
There was a problem hiding this comment.
Can we do this explicitly in that toLbNames() func? It seems confusing to have IngressKeyFunc, KeyFunc and ToIngressKey..
There was a problem hiding this comment.
Done. I will move this logic to separate function with v2 namer changes. Also refactored original IngressKeyFunc to utils as it will not be used by namer any more.
k8s-ci-robot
added
the
approved
skmatti
force-pushed
the
keyfunc-fix
branch
2 times, most recently
from
7b67130
to
27a1e49
Compare
| // So, logging the error would be sufficient here. | ||
| if err != nil { | ||
| klog.Errorf("Cannot get key for Ingress %v/%v: %v", ing.Namespace, ing.Name, err) | ||
| } |
There was a problem hiding this comment.
Can we do:
if err != nil {
klog.Errorf("Cannot get key for Ingress %v/%v: %v", ing.Namespace, ing.Name, err)
continue
}
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MrHohn, skmatti 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 |
Cherry pick #881 into 1.7
Fixes #879