fix(zbchaos): retry SaaS detection and panic if it fails instead of falling back to Self-Managed mode #490
Conversation
|
Unfortunately this breaks test that depend on the fallback behavior. For example Seems to me that we can't fix #488 without breaking a lot of tests 😅 |
| LogInfo("Failed to retrieve SaaS CRD, fallback to self-managed mode. %v", err) | ||
| return false | ||
| LogError("Failed to check for SaaS CRD, can't proceed without knowing if this is SaaS or Self-Managed: %v", err) | ||
| panic(err) |
There was a problem hiding this comment.
Let's not panic here :) If you want we can take a look at it together tomorrow.
There was a problem hiding this comment.
But then what? We don't know if it's SaaS or not
There was a problem hiding this comment.
My point I wanted to make is the following:
I don't want to have the panics in the internal package, you can see it as the lib or backend. It would be same as a library in java just does exit(1), it is unexpected and not nice. For me, it would be ok if you have this in the commands (it is the actual user of the libs, the user-facing code, the last layer).
I would like to see us to return an error and handle this by the consumer of the method.
I think we have here three cases to distinguish: a) CRD exists - SaaS b) CRD not exist - SM c) error -> return error. If currently not existing also causes an error (we have to find out/check this I guess, then we need to somehow need to distinguish these error cases.
There was a problem hiding this comment.
Okay that part makes sense to me but I'm not sure how to handle the errors. We check for SaaS when building the k8sclient so very early on for every request. What can we do if that fails?
I have added a dependency on a generic exponential backoff library because I didn't feel like writing this on my own. If querying for the CR still fails after retries are exhausted, we panic instead of just falling back to Self-Managed mode. I think this is preferable over retrying forever.
Closes #488