Implement EtcdRestore #137
Conversation
Using the latest (v0.18.0) of go-cloud causes an import issue with an ambigious import. That version isn't required, so we use v0.17.0 instead.
JamesLaverack
requested review from
munnerz
and removed request for
adamhosier
There was a problem hiding this comment.
Thanks @JamesLaverack
That's a lot of new code and I probably haven't digested it all yet.
I spotted one or two typos and made a few suggestions, but I'd be happy for you to merge this and improve upon it in followup branches.
| // should use a URL scheme of the bucket provider. For example `s3://my-amazon-bucket` or | ||
| // `gs://my-google-cloud-bucket`. | ||
| // +kubebuilder:validation:MinLength=3 | ||
| // +kubebuilder:validation:MaxLength=222 |
There was a problem hiding this comment.
I guess this comes from https://cloud.google.com/storage/docs/naming. Maybe add that link here.
There was a problem hiding this comment.
It is, but on reflection I'm tempted to remove the validation as I can't assume this will hold true of all bucket systems that go-cloud supports.
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: restorePodName(peer), | ||
| Namespace: restore.Namespace, | ||
| OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(&restore, etcdv1alpha1.GroupVersion.WithKind("EtcdRestore"))}, |
There was a problem hiding this comment.
Consider using controller-runtime controllerutil.SetControllerReference instead.
There was a problem hiding this comment.
Can we pull this out into a later issue? If we change how we set owner references it'd like to do it everywhere at once (e.g., the cluster controller creating EtcdPeers, the peer controller creating EtcdClusters).
Because SetControllerReference can return an error, I'd also have to change all of the functions that create these resources to return an error too and implemented the associated handling code.
|
Parking this for now, will re-open as part of #146 |
I just squash rebased this down to a single commit, rather than keep the incredibly messy history.