improbable-eng / etcd-cluster-operator

A controller to deploy and manage etcd clusters inside of Kubernetes
MIT License
128 stars 35 forks source link

Move controllers and webhooks into internal/ #162

Closed wallrj closed 4 years ago

wallrj commented 4 years ago
wallrj commented 4 years ago

I know the Go project layout suggests this, but KubeBuilder has its own expected layout and won't work without it kubernetes-sigs/kubebuilder#1268. So this change would break future use of KubeBuilder.

Same argument applies if we move main.go to cmd/manager/main.go (which I'd assumed we would do). Here for example is what happens if you attempt to add a conversion webhook:

richard   internal  ~  projects  improbable-eng  etcd-cluster-operator  130  git diff
diff --git a/main.go b/main.go
index 1f1fd45..e8543ba 100644
--- a/main.go
+++ b/main.go
@@ -12,6 +12,7 @@ import (
        "sigs.k8s.io/controller-runtime/pkg/log/zap"

        etcdv1alpha1 "github.com/improbable-eng/etcd-cluster-operator/api/v1alpha1"
+       "github.com/improbable-eng/etcd-cluster-operator/controllers"
        "github.com/improbable-eng/etcd-cluster-operator/internal/controllers"
        "github.com/improbable-eng/etcd-cluster-operator/internal/etcd"
        "github.com/improbable-eng/etcd-cluster-operator/internal/version"
@@ -103,6 +104,10 @@ func main() {
                setupLog.Error(err, "unable to create controller", "controller", "EtcdBackupSchedule")
                os.Exit(1)
        }
+       if err = (&etcdv1alpha1.EtcdCluster{}).SetupWebhookWithManager(mgr); err != nil {
+               setupLog.Error(err, "unable to create webhook", "webhook", "EtcdCluster")
+               os.Exit(1)
+       }
        // +kubebuilder:scaffold:builder
        if os.Getenv("DISABLE_WEBHOOKS") != "" {
                setupLog.Info("Skipping webhook set up.")

This means that we can't clean up the main.go file, for example moving the manager setup logic to a function that can also be re-used in tests.

My preference is to diverge from the kubebuilder layout at this point and set things out cleanly. It'll always be possible to run the kubebuilder generation in isolation and copy the generated files into our preferred directory structure.

Even if there was an option to configure KubeBuilder to use a different layout, I think there's value in leaving it 'standard' so that someone unfamiliar with the project can onboard faster by reading the KubeBuilder documentation and examples.

I don't buy that. The Kubebuilder layout is unfamiliar. Better to stick to the standard structure.

I'll close this for now, but would like to revisit in future.

See also https://github.com/kubernetes-sigs/kubebuilder/issues/932#issuecomment-524975469