From 932099d3b24577fb2b5412454903043bdaa7d7e1 Mon Sep 17 00:00:00 2001
From: Dimitri Koshkin <dimitri.koshkin@gmail.com>
Date: Mon, 9 Oct 2023 10:04:18 -0700
Subject: [PATCH] feat: allow changing strategy from ApplyAlways to Reconcile
 (#20)

Allows us to migrate ClusterResourceSets to the upstream Reconcile strategy and use CAPI directly with the next release
---
 .../api/v1beta1/clusterresourceset_webhook.go | 15 +++++++---
 .../clusterresourceset_webhook_test.go        | 29 +++++++++++++++++++
 2 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/exp/addons/api/v1beta1/clusterresourceset_webhook.go b/exp/addons/api/v1beta1/clusterresourceset_webhook.go
index 61f8f6dc5c02..604a79c3fc5b 100644
--- a/exp/addons/api/v1beta1/clusterresourceset_webhook.go
+++ b/exp/addons/api/v1beta1/clusterresourceset_webhook.go
@@ -48,6 +48,8 @@ func (m *ClusterResourceSet) Default() {
 	// ClusterResourceSet Strategy defaults to ApplyOnce.
 	if m.Spec.Strategy == "" {
 		m.Spec.Strategy = string(ClusterResourceSetStrategyApplyOnce)
+	} else if m.Spec.Strategy == string(ClusterResourceSetStrategyApplyAlways) {
+		m.Spec.Strategy = string(ClusterResourceSetStrategyReconcile)
 	}
 }
 
@@ -98,10 +100,15 @@ func (m *ClusterResourceSet) validate(old *ClusterResourceSet) error {
 	}
 
 	if old != nil && old.Spec.Strategy != "" && old.Spec.Strategy != m.Spec.Strategy {
-		allErrs = append(
-			allErrs,
-			field.Invalid(field.NewPath("spec", "strategy"), m.Spec.Strategy, "field is immutable"),
-		)
+		// Allow changing from ApplyAlways (a strategy that was added in this fork) to Reconcile.
+		// ApplyAlways is an "alias" for Reconcile and migrating to Reconcile will enable us to stop using a fork.
+		if !(old.Spec.Strategy == string(ClusterResourceSetStrategyApplyAlways) &&
+			m.Spec.Strategy == string(ClusterResourceSetStrategyReconcile)) {
+			allErrs = append(
+				allErrs,
+				field.Invalid(field.NewPath("spec", "strategy"), m.Spec.Strategy, "field is immutable"),
+			)
+		}
 	}
 
 	if old != nil && !reflect.DeepEqual(old.Spec.ClusterSelector, m.Spec.ClusterSelector) {
diff --git a/exp/addons/api/v1beta1/clusterresourceset_webhook_test.go b/exp/addons/api/v1beta1/clusterresourceset_webhook_test.go
index 525300b0cc43..2c76bab85003 100644
--- a/exp/addons/api/v1beta1/clusterresourceset_webhook_test.go
+++ b/exp/addons/api/v1beta1/clusterresourceset_webhook_test.go
@@ -38,6 +38,23 @@ func TestClusterResourcesetDefault(t *testing.T) {
 	g.Expect(clusterResourceSet.Spec.Strategy).To(Equal(string(ClusterResourceSetStrategyApplyOnce)))
 }
 
+func TestClusterResourcesetDefaultWithClusterResourceSetStrategyApplyAlways(t *testing.T) {
+	g := NewWithT(t)
+	clusterResourceSet := &ClusterResourceSet{
+		Spec: ClusterResourceSetSpec{
+			Strategy: string(ClusterResourceSetStrategyApplyAlways),
+		},
+	}
+	defaultingValidationCRS := clusterResourceSet.DeepCopy()
+	defaultingValidationCRS.Spec.ClusterSelector = metav1.LabelSelector{
+		MatchLabels: map[string]string{"foo": "bar"},
+	}
+	t.Run("for ClusterResourceSet", utildefaulting.DefaultValidateTest(defaultingValidationCRS))
+	clusterResourceSet.Default()
+
+	g.Expect(clusterResourceSet.Spec.Strategy).To(Equal(string(ClusterResourceSetStrategyReconcile)))
+}
+
 func TestClusterResourceSetLabelSelectorAsSelectorValidation(t *testing.T) {
 	tests := []struct {
 		name      string
@@ -104,6 +121,18 @@ func TestClusterResourceSetStrategyImmutable(t *testing.T) {
 			newStrategy: "",
 			expectErr:   true,
 		},
+		{
+			name:        "when the Strategy has changed, but the old value was ApplyAlways",
+			oldStrategy: string(ClusterResourceSetStrategyApplyAlways),
+			newStrategy: string(ClusterResourceSetStrategyReconcile),
+			expectErr:   false,
+		},
+		{
+			name:        "when the Strategy has changed, but the old value was ApplyAlways and the new value is ApplyOnce",
+			oldStrategy: string(ClusterResourceSetStrategyApplyAlways),
+			newStrategy: string(ClusterResourceSetStrategyApplyOnce),
+			expectErr:   true,
+		},
 	}
 
 	for _, tt := range tests {