Introduce Kubeclient::ResourceNotFoundError

[kirs]

First of all, thanks a lot for creating and maintaining the gem. It helps us a lot in building Kubernetes tools and scripts.

I'd like to to add a distinction between the generic error (KubeException) and the specific "not found" error. Right now in my code I have to do:

    def self.delete_namespace(namespace)
      kubeclient.delete_namespace(namespace)
    rescue KubeException => e
      raise unless e.to_s.include?("not found")
    end

This would be much easier if I could rescue only ResourceNotFoundError.

cc @simon3z @KnVerey

[simon3z]

@moolitayer @cben can you review? (It seems backward compatible to me but maybe you can spot something).

[cben]

Looks safe.
👍 to exposing Not Found in some way, 👍 to nesting it under Kubeclient:: rather than top-level like KubeException.

The problem this partially solves (as did #221) is we eat up specific RestClient exception class, replacing them all with a single class. Are we gonna want to re-expose more RestClient exceptions this way? Is there a better way?

Could we maybe inherit from RestClient's own exception classes? Can't inherit from 2 classes in ruby, so KubeException would have to become a module. In some cases like NotFound, we could simply "tag" the original exception object in-place by err.extend(KubeException); in other cases, like when we parse 'message' JSON field, we would use our own class (that also includes KubeException module).
See https://blog.jayway.com/2011/05/25/ruby-an-exceptional-language/ about this "tag with a module" technique.

In any case, given that RestClient has close to 70 exception classes: https://gist.github.com/cben/892458b3ca8f8fa28d4962503fb446f0, I'd at least stick to its naming.
If we need our own class, I'd call it Kubeclient::Exceptions::NotFound.

[simon3z]

@kirs BTW you can improve your code in the description by using the error_code (rather that the string error).

    def self.delete_namespace(namespace)
      kubeclient.delete_namespace(namespace)
    rescue KubeException => e
      raise unless e.error_code == 404
    end

Anyway it could be that NotFound is a common enough use-case to deserve its own exception.
Any thoughts on this @moolitayer @enoodle? I think you would use that in SSA.
@cben I didn't understand if you're in favor or not? (Beside naming).

[moolitayer]

Thought the same about testing the error_code vs. the message.

I'm not sure how well this would scale in terms of different http error classes.

I generally liked the approach @grosser suggested in #195 since

• we have a general HttpException
• exceptions are in the same file (and should be in the same namespace, see below). Not hooked on file but some form of aggregation for all exceptions.

If we do decide to go this direction, I have a few nit pics:

• all our exceptions should be name spaced the same, or code catching them would look inconsistent[1]
• the name resource_not_found_error should be more consistent with kube_exception
• we would need a test of backward compatibility of the new raised exception
  (Don't put any time into fixing just yet @kirs)

@kirs thanks for including the example code, it's explains clearly what you are trying to achieve which is good 👍

[1] If i remember correctly #195 was supposed to do that, in a backward compatible manner? i.e add correct scoping but also still support the non scope param and deprecate it

[grosser]

FYI would also be nice to use a http library that does not need native extensions (or depends on other gems)

[simon3z]

FYI would also be nice to use a http library that does not need native extensions (or depends on other gems)

@grosser any suggestion?

[grosser]

we use faraday a lot the middleware layering is a nice way of adding reusable addons ... httpclient looks decent too and has no dependencies

…

On Tue, Mar 14, 2017 at 8:29 AM, Federico Simoncelli < ***@***.***> wrote: FYI would also be nice to use a http library that does not need native extensions (or depends on other gems) @grosser <https://github.com/grosser> any suggestion? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#233 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAsZz_HCygRKCjGnHfagy_b8YSFMgeXks5rlrJjgaJpZM4MZoZO> .

[moolitayer]

we use faraday a lot the middleware layering is a nice way of adding
reusable addons ... httpclient looks decent too and has no dependencies

@grosser can you please create an issue with this info? we can then get more feedback on it (and we would also want to switch clients in some other gems)

[grosser]

rest-client issue moved to #237

[kirs]

Personally I don't think that there's any need for exception class per HTTP code. 404 is most common one, and for the rest it's enough to look into error.response.code.

the name resource_not_found_error should be more consistent with kube_exception

There's a convention in Ruby to name exception classes as "errors". Good examples are StandardError, ArgumentError, IndexError. Sometimes the error keyword is even omitted, like in case with ActiveRecord::RecordNotFound. If we want to bring consistency here, KubeException should be the one to rename.

Should I rebase my PR on top of #195?

[cben]

#195 has landed, please rebase (and inherit from the new Kubeclient::HttpError).
LGTM otherwise.

[kirs]

@cben thanks, I rebased it.

[moolitayer]

Patch looks good, feature seems reasonable.
cc @abonas @simon3z