Reform update strategy types

It's convenient to be able to leave out the update strategy, since
there is only one possible value at present; and if there were
alternatives, the present choice would still be a reasonable
default. However, with the format as it is, this doesn't work with
OpenAPIv3 schema, so you have to supply a value, even though there are
no parameters:

```yaml
spec:
  update:
    setters: {}
```

A more self-explanatory format which _does_ work with defaulting is to
name the strategy rather than relying on the presence of a field:

```yaml
spec:
  update:
    strategy: Setters
```

The whole `update` field can be elided and left to default. This
doesn't preclude having other strategies later, even those with
parameters, e.g.,

```yaml
spec:
  update:
    strategy: Foo
    fooParam: 5
```

This commit changes the API types and code that uses them, and the CRD
manifest, and adds a test that checks the defaulting actually works.

Signed-off-by: Michael Bridgen <michael@weave.works>

api/v1alpha1/imageupdateautomation_types.go

@@ -36,9 +36,10 @@ type ImageUpdateAutomationSpec struct {
 	// +required
 	Interval metav1.Duration `json:"interval"`
 	// Update gives the specification for how to update the files in
-	// the repository
+	// the repository. This can be left empty, to use the default
-	// +required
+	// value.
-	Update UpdateStrategy `json:"update"`
+	// +kubebuilder:default={"strategy":"Setters"}
+	Update *UpdateStrategy `json:"update,omitempty"`
 	// Commit specifies how to commit to the git repo
 	// +required
 	Commit CommitSpec `json:"commit"`
@@ -59,18 +60,25 @@ type GitCheckoutSpec struct {
 	Branch string `json:"branch"`
 }
 
-// UpdateStrategy is a union of the various strategies for updating
+// UpdateStrategyName is the type for names that go in
-// the git repository.
+// .update.strategy. NB the value in the const immediately below.
-type UpdateStrategy struct {
+// +kubebuilder:validation:Enum=Setters
-	// Setters if present means update workloads using setters, via
+type UpdateStrategyName string
-	// fields marked in the files themselves.
-	// +optional
-	Setters *SettersStrategy `json:"setters,omitempty"`
-}
 
-// SettersStrategy specifies how to use kyaml setters to update the
+const (
-// git repository.
+	// UpdateStrategySetters is the name of the update strategy that
-type SettersStrategy struct {
+	// uses kyaml setters. NB the value in the enum annotation for the
+	// type, above.
+	UpdateStrategySetters UpdateStrategyName = "Setters"
+)
+
+// UpdateStrategy is a union of the various strategies for updating
+// the Git repository. Parameters for each strategy (if any) can be
+// inlined here.
+type UpdateStrategy struct {
+	// Strategy names the strategy to be used.
+	// +required
+	Strategy UpdateStrategyName `json:"strategy"`
 }
 
 // CommitSpec specifies how to commit changes to the git repository

api/v1alpha1/zz_generated.deepcopy.go

@@ -120,7 +120,11 @@ func (in *ImageUpdateAutomationSpec) DeepCopyInto(out *ImageUpdateAutomationSpec
 	*out = *in
 	out.Checkout = in.Checkout
 	out.Interval = in.Interval
-	in.Update.DeepCopyInto(&out.Update)
+	if in.Update != nil {
+		in, out := &in.Update, &out.Update
+		*out = new(UpdateStrategy)
+		**out = **in
+	}
 	out.Commit = in.Commit
 }
 
@@ -165,29 +169,9 @@ func (in *ImageUpdateAutomationStatus) DeepCopy() *ImageUpdateAutomationStatus {
 	return out
 }
 
-// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
-func (in *SettersStrategy) DeepCopyInto(out *SettersStrategy) {
-	*out = *in
-}
-
-// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new SettersStrategy.
-func (in *SettersStrategy) DeepCopy() *SettersStrategy {
-	if in == nil {
-		return nil
-	}
-	out := new(SettersStrategy)
-	in.DeepCopyInto(out)
-	return out
-}
-
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *UpdateStrategy) DeepCopyInto(out *UpdateStrategy) {
 	*out = *in
-	if in.Setters != nil {
-		in, out := &in.Setters, &out.Setters
-		*out = new(SettersStrategy)
-		**out = **in
-	}
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new UpdateStrategy.

config/crd/bases/image.toolkit.fluxcd.io_imageupdateautomations.yaml

@@ -90,19 +90,24 @@ spec:
                   until it is unset (or set to false). Defaults to false.
                 type: boolean
               update:
+                default:
+                  strategy: Setters
                 description: Update gives the specification for how to update the
-                  files in the repository
+                  files in the repository. This can be left empty, to use the default
+                  value.
                 properties:
-                  setters:
+                  strategy:
-                    description: Setters if present means update workloads using setters,
+                    description: Strategy names the strategy to be used.
-                      via fields marked in the files themselves.
+                    enum:
-                    type: object
+                    - Setters
+                    type: string
+                required:
+                - strategy
                 type: object
             required:
             - checkout
             - commit
             - interval
-            - update
             type: object
           status:
             description: ImageUpdateAutomationStatus defines the observed state of

config/samples/image_v1alpha1_imageupdateautomation.yaml

@@ -3,5 +3,12 @@ kind: ImageUpdateAutomation
 metadata:
   name: imageupdateautomation-sample
 spec:
-  # Add fields here
+  checkout:
-  foo: bar
+    gitRepositoryRef:
+      name: app-repo
+    branch: main
+  interval: 5m
+  # update strategy is left to default to "Setters"
+  commit:
+    authorName: Fluxbot
+    authorEmail: fluxbot@example.com

controllers/imageupdateautomation_controller.go

@@ -168,9 +168,8 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(ctx context.Context, req ctr
 
 	log.V(debug).Info("cloned git repository", "gitrepository", originName, "branch", auto.Spec.Checkout.Branch, "working", tmp)
 
-	updateStrat := auto.Spec.Update
 	switch {
-	case updateStrat.Setters != nil:
+	case auto.Spec.Update != nil && auto.Spec.Update.Strategy == imagev1.UpdateStrategySetters:
 		// For setters we first want to compile a list of _all_ the
 		// policies in the same namespace (maybe in the future this
 		// could be filtered by the automation object).
@@ -185,8 +184,8 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(ctx context.Context, req ctr
 	default:
 		log.Info("no update strategy given in the spec")
 		// no sense rescheduling until this resource changes
-		r.event(ctx, auto, events.EventSeverityInfo, "no update strategy in spec, failing trivially")
+		r.event(ctx, auto, events.EventSeverityInfo, "no known update strategy in spec, failing trivially")
-		imagev1.SetImageUpdateAutomationReadiness(&auto, metav1.ConditionFalse, imagev1.NoStrategyReason, "no update strategy is given for object")
+		imagev1.SetImageUpdateAutomationReadiness(&auto, metav1.ConditionFalse, imagev1.NoStrategyReason, "no known update strategy is given for object")
 		err := r.Status().Update(ctx, &auto)
 		return ctrl.Result{}, err
 	}

controllers/update_test.go

@@ -182,6 +182,48 @@ var _ = Describe("ImageUpdateAutomation", func() {
 			Expect(k8sClient.Delete(context.Background(), policy)).To(Succeed())
 		})
 
+		Context("defaulting", func() {
+			var key types.NamespacedName
+			var auto *imagev1.ImageUpdateAutomation
+
+			BeforeEach(func() {
+				key = types.NamespacedName{
+					Namespace: gitRepoKey.Namespace,
+					Name:      "update-" + randStringRunes(5),
+				}
+				auto = &imagev1.ImageUpdateAutomation{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      key.Name,
+						Namespace: key.Namespace,
+					},
+					Spec: imagev1.ImageUpdateAutomationSpec{
+						Interval: metav1.Duration{Duration: 2 * time.Hour}, // this is to ensure any subsequent run should be outside the scope of the testing
+						Checkout: imagev1.GitCheckoutSpec{
+							GitRepositoryRef: corev1.LocalObjectReference{
+								Name: "garbage",
+							},
+							Branch: branch,
+						},
+						// leave Update field out
+						Commit: imagev1.CommitSpec{
+							MessageTemplate: commitMessage,
+						},
+					},
+				}
+				Expect(k8sClient.Create(context.Background(), auto)).To(Succeed())
+			})
+
+			AfterEach(func() {
+				Expect(k8sClient.Delete(context.Background(), auto)).To(Succeed())
+			})
+
+			It("defaults .spec.update to {strategy: Setters}", func() {
+				var fetchedAuto imagev1.ImageUpdateAutomation
+				Expect(k8sClient.Get(context.Background(), key, &fetchedAuto)).To(Succeed())
+				Expect(fetchedAuto.Spec.Update).To(Equal(&imagev1.UpdateStrategy{Strategy: imagev1.UpdateStrategySetters}))
+			})
+		})
+
 		Context("with Setters", func() {
 
 			var (
@@ -220,8 +262,8 @@ var _ = Describe("ImageUpdateAutomation", func() {
 							},
 							Branch: branch,
 						},
-						Update: imagev1.UpdateStrategy{
+						Update: &imagev1.UpdateStrategy{
-							Setters: &imagev1.SettersStrategy{},
+							Strategy: imagev1.UpdateStrategySetters,
 						},
 						Commit: imagev1.CommitSpec{
 							MessageTemplate: commitMessage,