Implementing Apply, Delete and DeleteByName in Kube HTTP Client #610
Conversation
luizbafilho
changed the title
…nto 546-kube-apply
| if err != nil { | ||
| return nil, fmt.Errorf("failed to initialize discovery client: %s", err) | ||
| } | ||
| mapper := restmapper.NewDeferredDiscoveryRESTMapper(memory.NewMemCacheClient(dc)) |
There was a problem hiding this comment.
Is there a memcached instance running somewhere? Or is this only an in-memory cache?
There was a problem hiding this comment.
That in-memory cache only I believe
indeed, that ":8080" I reported happened while implementing those tests. |
Might help, it has a teardown func: weave-gitops/pkg/testutils/testutils.go Line 18 in 6b91a81 |
pkg/kube/kubehttp.go
Outdated
|
|
||
| func (c *KubeHTTP) getResourceInterface(manifest []byte) (dynamic.ResourceInterface, string, []byte, error) { | ||
| obj := &unstructured.Unstructured{} | ||
| _, gvk, err := decUnstructured.Decode([]byte(manifest), nil, obj) |
There was a problem hiding this comment.
Major: It's already a []byte. Why create a new one?
pkg/kube/kubehttp_test.go
Outdated
| Expect(kustObj.Name).To(Equal(name)) | ||
| }) | ||
|
|
||
| It("Delete", func() { |
There was a problem hiding this comment.
Major: I'll make this comment here and at the top of the PR. I would really like to see more thorough testing on apply, delete, delete by name. This is a super crucial object and I think it should be locked down tight.
Some other tests I would like to see are:
- Better error handling (i.e. test the error path and confirm those areas are covered)
- Test namespace scoped objects and non-namespace scoped objects. The current tests just test namespace resources.
- Malformed manifests being passed into the functions
An example, is what happens to Apply when a different namespace is passed in as the parameter and the manifest parameter has a different namespace.
I know as a team we haven't come together yet on what are expectations for unit testing. Happy to discuss this further face-to-face or pair program to get some extra tests down for these functions.
| default: | ||
| return kube.GVRKustomization | ||
| return schema.GroupVersionResource{}, fmt.Errorf("no matching schema.GroupVersionResource to the ResourceKind: %s", string(rk)) |
There was a problem hiding this comment.
FYI: I think it's good that you are passing the resourceKind that caused the error. Another approach could have been a custom error:
type ConversionError struct {
kind string
}
func (e *ConversionError) Error() string {
return fmt.Sprintf("no matching schema.GroupVersionResource to the ResourceKind: %s", string(e.kind))
}
The effect of this is a more clear way to check for the error consistently with errors.As(err, &e) and not having to worry about the ResourceKind changing on you. However, this is excessive considering you aren't explicitly checking this error and handling it in a special way.
Thanks for addressing my main concern of odd side effects.
What changed?
Apply,DeleteandDeleteByNamein kubehttp #546Why?
kubectlbinaryHow did you test it?
Release notes
Documentation Changes