kubernetes/Controller: Fix return type during initialization #1705
Conversation
Fixes the return value and signature when a new Kubernetes Controller is being initialized by returning the interface and an error.
| kubernetesClient := k8s.NewKubernetesClient(kubeClient, meshName, stop) | ||
| kubernetesClient, err := k8s.NewKubernetesClient(kubeClient, meshName, stop) | ||
| if err != nil { | ||
| log.Fatal().Err(err).Msg("Error initializing kubernetes controller") |
There was a problem hiding this comment.
Instead of log.Fatal which calls os.Exit(1), could we use GinkgoT().Fatal() or something similar so the failure is registered by the testing framework?
There was a problem hiding this comment.
Not familiar with Ginko's fatal, but seems like we should leverage this across the test code. Would you mind creating an issue to handle this repo wide?
There was a problem hiding this comment.
What I understand is GinkgoT() is just a way to get a handle to the current *testing.T, so it's basically the equivalent of t.Fatal(). Yes, will open an issue.
| // NewKubernetesClient returns a new Client which means to provide access to locally-cached k8s resources | ||
| func NewKubernetesClient(kubeClient kubernetes.Interface, meshName string, stop chan struct{}) Client { | ||
| // NewKubernetesClient returns a new kubernetes.Controller which means to provide access to locally-cached k8s resources | ||
| func NewKubernetesClient(kubeClient kubernetes.Interface, meshName string, stop chan struct{}) (Controller, error) { |
There was a problem hiding this comment.
Does changing the name of this function to NewKubernetesController make it any more clear?
There was a problem hiding this comment.
I thought about this, but kept it as is since a few other interfaces are instantiated using NewXXXClient. How about I handle these renames generically in a follow-up?
Fixes the return value and signature when a new Kubernetes
Controller is being initialized by returning the interface
and an error.
Please answer the following questions with yes/no.
No