Test an api group or specific resource kind is registered

[cben]

With custom resources, whether by CRD or API agregration, it's increasingly common not to be sure the API group you expect is there. If the CRD is not yet defined, or the API server is down, you get an error.

• [x] Perhaps detecting whole API group missing is already possible — create Client for it, call .discover, rescue excepion?
• [x] For testing a single resource kind, can you rely on NoMethodError / .respond_to? ?
• [ ] Document this.

[cben]

https://github.com/ManageIQ/manageiq-providers-kubernetes/pull/284 confirms .discover raises if whole api group doesn't exist. Moving discussion from there:

@cben there's probably a reason for why it is the way it is but what would you think about kubeclient doing .discover when connecting? That way if the service isn't setup or is down for some reason the connect would actually fail (which at least to me seems more correct) rather than on the first method call?

@agrare great question. Originally kubeclient hardcoded supported entities, so no network communication was needed during Client.new; when discovery was discussed #144 & implemented #185, I don't see direct explanation why discovery was made "lazy". My guesses:

1. Backward compatibility. lazy discovery is closer to original behavior that .new wouldn't raise network errors, only subsequent .get_foos etc.

2. Optimization — creating Client and not using it doesn't use network.

3. It would be somewhat surprising if Client.new would have network side effects (and could cause exceptions dependent on external factors), constructors usually don't. Now, it makes sense for a network client object to connect immediately, though then it'd be better named say Client.connect factory and not .new. OTOH, a Client instance plays 2 roles — represents a connection, but also represents settings about how to connect. It this light, it's convenient to be able to cheaply prepare say 5 client objects in one part of the code and pass them around, leaving it to parts of the code that actually do actions to choose whether/which of them to use...

[cben]

About changing .new: IMHO not worth backward compatibility price. We already have .discover.

Could add a factory method that does immediate discovery like Kubeclient::Client.connect but not even sure it's worth a new method. Also, in case of https://github.com/ManageIQ/manageiq-providers-kubernetes/pull/284 that project has manager.connect eventually wrapping Kubeclient::Client.new — do we always want it to fail fast, and are willing to rewrite to use Kubeclient::Client.connect? Or only in this place?

@agrare what do you think about changing .discover to return self, so it's easier to write client = Kubeclient::Client.new(...).discover (or client = whatever_helper_returning_client(...).discover)?

[agrare]

@cben I didn't catch that we were just calling Kubeclient::Client.new from ContainerManagerMixin#connect, I completely agree that the simple constructor should not do any network calls.

I like the idea of a Kubeclient::Client.connect which would call new and discovery. This is very similar to RbVmomi's VIM.connect method.

We could also just simulate that by moving the discover into ContainerManagerMixin#connect.

[agrare]

The respond_to? is interesting as well, currently it isn't possible to do something like this:

kube = Kubeclient::Client.new()
kube.get_cluster_service_classes if kube.respond_to?(:get_cluster_service_classes)

because it will raise a 404 exception instead of returning false.

Maybe we could catch exceptions raised by discover here and here so that respond_to? would work?

[cben]

Hmm, we do have unit tests for respond_to? working: https://github.com/abonas/kubeclient/blob/master/test/test_missing_methods.rb (It even triggers immediate discovery if not done yet. It's a bit surprising compared to "constructor doesn't do any network call" but we have no choice...)

When API is responding this works for me (without service broker's custom resources):

[6] pry(main)> client.respond_to?(:get_cluster_service_classes)
=> false

Do you mean if discovery fails with error?

[5] pry(main)> client.respond_to?(:get_cluster_service_classes)
Errno::ECONNREFUSED: Failed to open TCP connection to 127.0.0.1:8443 (Connection refused - connect(2) for "127.0.0.1" port 8443)

Is it legitimate for respond_to? to raise errors in Ruby? But I'm not sure it's sensible to coerce this to false or true — it's more of a "don't know" (and client.discovered remains false, so it'll retry discovery on future method calls and respond_to? attempts).

[agrare]

Is it legitimate for respond_to? to raise errors in Ruby?

I don't see anything in the docs that states that respond_to? can't raise an exception but I certainly wouldn't expect it.

But I'm not sure it's sensible to coerce this to false or true — it's more of a "don't know" (and client.discovered remains false, so it'll retry discovery on future method calls and respond_to? attempts).

That's fair, so if discovery fails we should leave client.discovered as false so it would be retried? That works for me.

[cben]

That's fair, so if discovery fails we should leave client.discovered as false so it would be retried?

Yes, this already happens. But then, whether the original response was assumed true or false, it might have been a lie — if later discovery succeeds, it will change.