Initial Contour Controller Implementation #10
Conversation
|
/assign @jpeach |
controller/contour/controller.go
Outdated
| // so we can clean up later. | ||
| if !slice.ContainsString(contour.Finalizers, contourFinalizer) { | ||
| updated := contour.DeepCopy() | ||
| updated.Finalizers = append(updated.Finalizers, contourFinalizer) |
There was a problem hiding this comment.
The kubebuilder finalizer example suggests checking ObjectMeta.DeletionTimestamp.IsZero() before adding the finalizer; is there a reason to omit that here?
controller/contour/controller.go
Outdated
|
|
||
| // The contour is safe to process, so ensure current state matches desired state. | ||
| desired := true | ||
| if err := r.ensureContour(desired, contour); err != nil { |
There was a problem hiding this comment.
This, and the ensureNamespace below almost suggest a table-driven approach to me, e.f.:
steps:= []func(context.Context, *Reconciler) error {
EnsureContourFinalizer,
EnsureContourNamespace,
....
}Not sure if this is a common pattern for this sort of thing though.
There was a problem hiding this comment.
ensureContour is the func that will encapsulate all the lower-level funcs required to ensure dependent resources exist. A mirrored workflow will be created for removing contour and dependent resources. I've been working through the changes and will submit a new commit shortly. The pattern I'm trying to establish is similar to other operators and upstream k8s controllers. I'm open to feedback, so let me know your thoughts after I reviewing my upcoming commit.
controller/contour/controller.go
Outdated
| "github.com/projectcontour/contour-operator/util/slice" | ||
| "k8s.io/apimachinery/pkg/api/errors" | ||
|
|
||
| operatorv1alpha1 "github.com/projectcontour/contour-operator/api/v1alpha1" |
There was a problem hiding this comment.
Personally, I prefer import in 3 blocks - stdlib, local, vendor. I think that 2 blocks (stdlib, everything else) is the most common approach though, so let's do that.
There was a problem hiding this comment.
I'm for stdlib, local, vendor. I'll clean up the imports with my upcoming commit.
There was a problem hiding this comment.
Cool, I believe that goimports has a -local flag that may help with this; maybe we can add that to the lint config :)
controller/contour/controller.go
Outdated
| } | ||
|
|
||
| // ensureContour ensures all necessary resources exist for the given contour. | ||
| func (r *Reconciler) ensureContour(desired bool, contour *operatorv1alpha1.Contour) error { |
There was a problem hiding this comment.
Pass the caller context down?
| expectAfter: time.Second * 15, | ||
| }, | ||
| } | ||
| for _, test := range tests { |
There was a problem hiding this comment.
You can use subtests here:
cases := map[string]struct{ ... } { ... }
for name, case := range cases {
t.Run(name, func(t *testing.T) {
err := NewMaybeRetryableAggregate(case.errors)
...
}
}
controller/contour/controller.go
Outdated
| } | ||
|
|
||
| // The contour is safe to process, so ensure current state matches desired state. | ||
| desired := true |
There was a problem hiding this comment.
I guess it's ok for now, but
- passing a bool is harder to read and understand than an enum
- teardown probably needs to be done in the reverse order
There was a problem hiding this comment.
Yeah, I refactored based on #10 (comment).
controller/contour/namespace.go
Outdated
| ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: name}} | ||
| if err := r.Get(context.TODO(), types.NamespacedName{Name: ns.Name}, ns); err != nil { | ||
| if !errors.IsNotFound(err) { | ||
| return nil, fmt.Errorf("failed to get namespace %s: %v", ns.Name, err) |
There was a problem hiding this comment.
This isn't an error if we are tearing down the namespace, though we wold want to update status in that case (maybe just add a TODO here?)
15d8223
to
1d13240
Compare
Signed-off-by: Daneyon Hansen <[email protected]>
|
@jpeach I believe commit |
main()to cmd/operator pkg.cmd/operator/start.go: Exposes a flag to set the Contour image used by the operator.controller/contour/controller.go: Initial controller implementation that watches the contour CRD and a) finalizes the resource and b) creates the contour namespace if it does not exist.util/retryableerror/retryableerror.go: Utility function for retrying reconciliation errors.util/slice/slice.go: Utility function to check if a given slice of strings contains the provided string.Signed-off-by: Daneyon Hansen [email protected]