Proposed #3
Proposed #3
Conversation
gardener-robot-ci-3
added
the
reviewed/ok-to-test
gardener-robot
added
needs/review
There was a problem hiding this comment.
Thanks for the PR 🚀
I finished my first review round and found a couple of things.
Other than that, the license headers are missing in all *.go files.
It might also make sense to create a linter Make target right from the beginning. You could reuse the check and format make targets. The corresponding tools are installed via tools.mk.
Another thing which came to my mind. In gardener/gardener we just removed the vendorfolder (ref) and we plan to continue in extensions too. As this is a new component, it would be sense to not create it in the first place.
| // WebhookCertDir is the directory that contains the webhook server key and certificate. | ||
| WebhookCertDir string |
There was a problem hiding this comment.
Covered by the previous answer.
| // GetClientSet returns a [kubernetes.Clientset] from the given kubeconfig path. | ||
| // If kubeconfigPath is nil, the function assumes that the process is running in a K8s pod, and attempts to create | ||
| // client set based on the convention for in-cluster configuration. | ||
| func GetClientSet(kubeconfigPath string) (*kclient.Clientset, error) { |
There was a problem hiding this comment.
Why do we need such a function? When using controller-runtime the manager creates a client/clientset based on a rest.Config.
There was a problem hiding this comment.
It is only used to validate the connection settings. A clientset is created, just to see if there would be errors, and then discarded. I saw that approach in another Gardener project. I don't know if it is really necessary - my guess was that it was added to compensate for some case where the manager-issued error message was too unclear.
| "sigs.k8s.io/controller-runtime/pkg/webhook" | ||
| ) | ||
|
|
||
| type FakeManager struct { |
There was a problem hiding this comment.
We typically don't have a FakeManager in Gardener.
For unit tests normal fake clients should be enough. For integration tests we use sigs.k8s.io/controller-runtime/pkg/envtest where the real manager could be used.
There was a problem hiding this comment.
How does one achieve isolation during unit tests, for e.g. a function which uses a manager to run a controller? In case I want to verify that the function will make an attempt to run the controller, but I don't want the controller actually executed.
I'd like to leave that for last, so I do all of them in one go and don't risk some file slipping through the cracks. Do we have an automatic check to save me, in case I forget adding a copyright header?
Done. I have a Regarding removing the vendor folder: |
gardener-robot
added
the
lifecycle/stale
Initial implementation of the HA service, which is responsible for ensuring HA operation by injecting a service endpoint directing traffic to the active replica. This initial PR also adds most of the component's central infrastructure.