From 72cd9b83d6bdf9d614792fb51422d584a345f6e2 Mon Sep 17 00:00:00 2001
From: natasha41575 <natashasarkar@google.com>
Date: Thu, 23 Feb 2023 17:09:08 +0000
Subject: [PATCH] add sample and address code review comments

---
 .../gitops_rollout_deletionpolicy.yaml        | 52 +++++++++++++++++++
 rollouts/controllers/rollout_controller.go    | 22 +++++---
 2 files changed, 67 insertions(+), 7 deletions(-)
 create mode 100644 rollouts/config/samples/gitops_rollout_deletionpolicy.yaml

diff --git a/rollouts/config/samples/gitops_rollout_deletionpolicy.yaml b/rollouts/config/samples/gitops_rollout_deletionpolicy.yaml
new file mode 100644
index 0000000000..b36bd266a5
--- /dev/null
+++ b/rollouts/config/samples/gitops_rollout_deletionpolicy.yaml
@@ -0,0 +1,52 @@
+# Copyright 2022 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Adding 'configsync.gke.io/deletion-propagation-policy: Foreground' will result in
+# managed objects being cleaned up by Config Sync when the RootSync object created
+# by the rollout is deleted. As a note, this is not included in any release tag (as
+# of the release date of v1.14.2), and to use this, you must build Config Sync from
+# HEAD.
+
+apiVersion: gitops.kpt.dev/v1alpha1
+kind: Rollout
+metadata:
+  name: sample
+spec:
+  description: rollout kpt samples
+  clusters:
+    sourceType: KCC
+  packages:
+    sourceType: GitHub
+    github:
+      selector:
+        org: GoogleContainerTools
+        repo: kpt-samples
+        directory: "*"
+        revision: main
+  targets:
+    selector:
+      matchLabels:
+        location/city: example
+  packageToTargetMatcher:
+    type: AllClusters
+  strategy:
+    type: RollingUpdate
+    rollingUpdate:
+      maxConcurrent: 2
+  syncTemplate:
+    rootSync:
+      metadata:
+        annotations:
+          configsync.gke.io/deletion-propagation-policy: Foreground
+    type: RootSync
\ No newline at end of file
diff --git a/rollouts/controllers/rollout_controller.go b/rollouts/controllers/rollout_controller.go
index 28218ea297..603075caf0 100644
--- a/rollouts/controllers/rollout_controller.go
+++ b/rollouts/controllers/rollout_controller.go
@@ -422,13 +422,9 @@ func (r *RolloutReconciler) computeTargets(ctx context.Context,
 			}
 		} else {
 			// remoterootsync already exists
-			if pkg.Revision != rrs.Spec.Template.Spec.Git.Revision ||
-				!reflect.DeepEqual(rollout.Spec.SyncTemplate.RootSync.Metadata,
-					rrs.Spec.Template.Metadata) {
-				rrs.Spec.Template.Spec.Git.Revision = pkg.Revision
-				rrs.Spec.Template.Metadata = rollout.Spec.SyncTemplate.RootSync.Metadata
-				// revision and/or metadata has been updated
-				targets.ToBeUpdated = append(targets.ToBeUpdated, &rrs)
+			updated, needsUpdate := pkgNeedsUpdate(rollout, rrs, pkg)
+			if needsUpdate {
+				targets.ToBeUpdated = append(targets.ToBeUpdated, updated)
 			} else {
 				targets.Unchanged = append(targets.Unchanged, &rrs)
 			}
@@ -442,6 +438,18 @@ func (r *RolloutReconciler) computeTargets(ctx context.Context,
 	return targets, nil
 }
 
+func pkgNeedsUpdate(rollout *gitopsv1alpha1.Rollout, rrs gitopsv1alpha1.RemoteRootSync, pkg *packagediscovery.DiscoveredPackage) (*gitopsv1alpha1.RemoteRootSync, bool) {
+	// TODO: We need to check other things here besides git.Revision and metadata
+	if pkg.Revision != rrs.Spec.Template.Spec.Git.Revision ||
+		!reflect.DeepEqual(rollout.Spec.SyncTemplate.RootSync.Metadata,
+			rrs.Spec.Template.Metadata) {
+		rrs.Spec.Template.Spec.Git.Revision = pkg.Revision
+		rrs.Spec.Template.Metadata = rollout.Spec.SyncTemplate.RootSync.Metadata
+		return &rrs, true
+	}
+	return nil, false
+}
+
 func (r *RolloutReconciler) getWaveTargets(ctx context.Context, rollout *gitopsv1alpha1.Rollout, allTargets *Targets, allClusters []clusterstore.Cluster,
 	allWaves []gitopsv1alpha1.Wave) ([]WaveTarget, error) {
 	allWaveTargets := []WaveTarget{}