Prevent blocking namespace deletion

[tralafiti]

Prevent blocking namespace deletion by yielding in chart handlers OnRemove callback if target namespace is in terminating phase.

Fixes #135

[brandond]

Question: rather than adding a new watch+cache on namespaces, would it be easier to handle the unable to create new content in namespace NS because it is being terminated error when applying the job? You can use apierrors.IsForbidden(err) && apierrors.HasStatusCause(err, corev1.NamespaceTerminatingCause) for this.

[tralafiti]

@brandond Thank you for the feedback! Inspecting the error was my first approach actually. Sadly it's an github.com/rancher/wrangler/v3/pkg/merr which apierrors didn't handle correctly. IMHO typecasting and iterating over the []error for the check was a bit cumbersome and just inspecting the namespace itself sounded cleaner to me. Also firing the jobs just to check if they failed felt wasteful. I'm not familiar with wrangler and would have expecting a just-in-time querying of the k8s resource and not an additional watch to be honest. Please let me know if you prefer the error-checking-solution and I will update the PR :)

[brandond]

You would need to do a type switch to see if the return is merr.Errors (type alias for []error) or just a single error - but I think that's preferable if it works.

While it may be a little more code, the alternative is adding a namespace cache, or doing an uncached lookup against the apiserver every time this OnRemove handler runs - which may be multiple times in short succession due to how wrangler retries handlers.

[tralafiti]

Replaced the commit with the discussed solution

[brandond]

LGTM - but does it work? Can you add a test for this to https://github.com/k3s-io/helm-controller/blob/master/test/suite/helm_test.go ?

Should be pretty easy to just copy one of the existing create/delete tests - just add a step in the middle to delete the ns.

[tralafiti]

Only checked the make file and didn't see the e2e tests in scripts, thanks for your hint. Added test coverage. FYI I had to change the controller args to start cluster scoped for this to be possible.

[brandond]

LGTM, tests look good - but please make sure all your commits have --sign-off for DCO.

[tralafiti]

Sorry, my muscle memory favors -S over -s 😅. Fixed