Make sure cache Reader still works #39
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
@pwittrock @droot I'll do some more tests (either here or in a follow-up PR), but here's enough to get started. |
pkg/cache/informer_cache.go
Outdated
| gvk, err := apiutil.GVKForObject(out, ip.Scheme) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| cache, err := ip.InformersMap.Get(gvk, cacheType) | ||
| // we need the non-list GVK, so chop off the "List" from the end of the kind | ||
| gvk.Kind = gvk.Kind[:len(gvk.Kind)-4] |
There was a problem hiding this comment.
strings.TrimSuffix(gvk.Kind, "List") is more safe for this use-case ?
There was a problem hiding this comment.
I'd argue that it's less correct. We have an invariant that you must pass a list type, and the list type must be of name FooList. We should either error out or panic if that's not the case, but failing silently (like what we do here, or your suggestion) is probably not the right answer.
pkg/cache/informer_cache.go
Outdated
| cacheTypeObj, ok := cacheTypeValue.Interface().(runtime.Object) | ||
| if !ok { | ||
| return fmt.Errorf("cannot get cache for %T, its element is not a runtime.Object", out) | ||
| } |
There was a problem hiding this comment.
Pl. add %T for cacheTypeValue in the error, will help in debugging.
|
@DirectXMan12 I can address the comments I posted in follow-up PR if you think you won't have time to get to it today. |
|
@DirectXMan12 Travis tests are also not passing. complaining about missing some pkgs. |
|
@DirectXMan12 @droot The errors should be legit now. Seem to be linting errors. |
DirectXMan12
force-pushed
the
bug/cache-list-reflection
branch
from
11eb5ac to
a94b0ce
Compare
|
@DirectXMan12 You will have to run "goimports -w" on informer_cache.go to fix the lint error thrown in travis test, then we are good to go. |
The cache refactor broke the cache reader implementation (invalid assumptions about names, arguments, and the way reflection works). This fixes it, and adds back in some working tests to ensure that it doesn't break again.
DirectXMan12
force-pushed
the
bug/cache-list-reflection
branch
from
a94b0ce to
9db07f6
Compare
|
man, that error message from the linter is terrible. It doesn't really say what the problem is. |
|
@DirectXMan12 Yes, that message it terrible, I ran in to the other day and it took me sometime to figure out what it really meant :) |
|
/approve |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
droot
added
the
approved
…t-reflection Make sure cache Reader still works
The cache refactor broke the cache reader implementation (invalid
assumptions about names, arguments, and the way reflection works).
This fixes it, and adds back in some working tests to ensure that
it doesn't break again.