correct delta_pre_compare for DBInstance

In PR #95, we added a `customPreCompare` function to
`pkg/resource/db_instance/hooks.go` and changed the `generator.yaml` to
use a code snippet instead of the
`templates/hooks/db_instance/delta_pre_compare.go.tpl` template file.
However, we did not delete the
`templates/hooks/db_instance/delta_pre_compare.go.tpl` file and when I
went to do the updating DB instance tags patch, mobody noticed that I
had added a call to `compareTags` into the (now-unused) template file.
This meant that `compareTags` is never called which means changes to
tags were not being properly on DBInstance resources.

This patch moves all the custom code for delta pre-checks back into the
template file and gets rid of the custom code snippet in generator.yaml,
consolidating all this logic into the template file.

Signed-off-by: Jay Pipes <[email protected]>

Author: jaypipes

apis/v1alpha1/ack-generate-metadata.yaml

@@ -1,13 +1,13 @@
 ack_generate_info:
-  build_date: "2022-08-04T21:32:49Z"
+  build_date: "2022-08-13T17:21:53Z"
   build_hash: fe61d04673fd4d9848d5f726b01e0689a16d3733
   go_version: go1.18.1
   version: v0.19.3-1-gfe61d04
 api_directory_checksum: 6a967cc8a62d521d4f4816dbccc48f81d0cb271d
 api_version: v1alpha1
 aws_sdk_go_version: v1.44.27
 generator_config_info:
-  file_checksum: dc22b2b5295201b27341f95c1cec0b0dcead98b3
+  file_checksum: 48d8a87d42545d99ddabf9bea4ab180e08ab79b7
   original_file_name: generator.yaml
 last_modification:
   reason: API generation

apis/v1alpha1/generator.yaml

@@ -183,6 +183,8 @@ resources:
           is_ignored: true
   DBInstance:
     hooks:
+      delta_pre_compare:
+        template_path: hooks/db_instance/delta_pre_compare.go.tpl
       sdk_create_pre_build_request:
         template_path: hooks/db_instance/sdk_create_pre_build_request.go.tpl
       sdk_create_post_set_output:
@@ -199,8 +201,6 @@ resources:
         template_path: hooks/db_instance/sdk_delete_pre_build_request.go.tpl
       sdk_file_end:
         template_path: hooks/db_instance/sdk_file_end.go.tpl
-      delta_pre_compare:
-        code: customPreCompare(a, b)
     exceptions:
       terminal_codes:
         - InvalidParameter

generator.yaml

@@ -183,6 +183,8 @@ resources:
           is_ignored: true
   DBInstance:
     hooks:
+      delta_pre_compare:
+        template_path: hooks/db_instance/delta_pre_compare.go.tpl
       sdk_create_pre_build_request:
         template_path: hooks/db_instance/sdk_create_pre_build_request.go.tpl
       sdk_create_post_set_output:
@@ -199,8 +201,6 @@ resources:
         template_path: hooks/db_instance/sdk_delete_pre_build_request.go.tpl
       sdk_file_end:
         template_path: hooks/db_instance/sdk_file_end.go.tpl
-      delta_pre_compare:
-        code: customPreCompare(a, b)
     exceptions:
       terminal_codes:
         - InvalidParameter

pkg/resource/db_instance/delta.go

@@ -384,45 +384,6 @@ func (rm *resourceManager) createDBInstanceReadReplica(
 	return &resource{r.ko}, nil
 }
 
-func customPreCompare(a *resource, b *resource) {
-	// Do not consider any of the following fields for delta if they are missing in
-	// desired(a) but are present in latest(b) because each of these fields is
-	// late-initialized
-	// This special handling is only needed for DBInstance because late
-	// initialized values are not returned after successful ModifyDBInstance
-	// call. They are only populated once the DBInstance returns back to
-	// available.
-	if a.ko.Spec.AvailabilityZone == nil &&
-		b.ko.Spec.AvailabilityZone != nil {
-		a.ko.Spec.AvailabilityZone = b.ko.Spec.AvailabilityZone
-	}
-	if a.ko.Spec.BackupTarget == nil &&
-		b.ko.Spec.BackupTarget != nil &&
-		*b.ko.Spec.BackupTarget == ServiceDefaultBackupTarget {
-		a.ko.Spec.BackupTarget = b.ko.Spec.BackupTarget
-	}
-	if a.ko.Spec.NetworkType == nil &&
-		b.ko.Spec.NetworkType != nil &&
-		*b.ko.Spec.NetworkType == ServiceDefaultNetworkType {
-		a.ko.Spec.NetworkType = b.ko.Spec.NetworkType
-	}
-	if a.ko.Spec.PerformanceInsightsRetentionPeriod == nil &&
-		b.ko.Spec.PerformanceInsightsRetentionPeriod != nil &&
-		*b.ko.Spec.PerformanceInsightsRetentionPeriod == ServiceDefaultInsightsRetentionPeriod {
-		a.ko.Spec.PerformanceInsightsRetentionPeriod = b.ko.Spec.PerformanceInsightsRetentionPeriod
-	}
-	if a.ko.Spec.PerformanceInsightsKMSKeyID == nil &&
-		b.ko.Spec.PerformanceInsightsKMSKeyID != nil {
-		a.ko.Spec.PerformanceInsightsKMSKeyID = b.ko.Spec.PerformanceInsightsKMSKeyID
-	}
-
-	// RDS will choose preferred engine minor version if only
-	// engine major version is provided and controler should not
-	// treat them as different, such as spec has 14, status has 14.1
-	// controller should treat them as same
-	reconcileEngineVersion(a, b)
-}
-
 // RDS will choose preferred engine minor version if only
 // engine major version is provided and controler should not
 // treat them as different, such as spec has 14, status has 14.1

pkg/resource/db_instance/hooks.go

@@ -1,13 +1,37 @@
-    // Do not consider AvailabilityZone field for delta if it is missing in
-    // desired(a) and is present in latest(b) because AvailabilityZone field is
-    // late-initialized
-    // This special handling is only needed for DBInstance because a requeue
-    // error needs to be returned even after successful ModifyDBInstance call.
-    // See sdk_update_post_set_output.go.tpl for more details.
-    // The requeue error from update prevents the late initialization in
-    // reconciler.go and without ignoring AvailabilityZone for delta , the
-    // reconciler will keep updating the dbinstance and constantly requeueing it.
-    if a != nil && a.ko.Spec.AvailabilityZone == nil && b != nil && b.ko.Spec.AvailabilityZone != nil {
-        a.ko.Spec.AvailabilityZone = b.ko.Spec.AvailabilityZone
-    }
+	// Do not consider any of the following fields for delta if they are missing in
+	// desired(a) but are present in latest(b) because each of these fields is
+	// late-initialized
+	// This special handling is only needed for DBInstance because late
+	// initialized values are not returned after successful ModifyDBInstance
+	// call. They are only populated once the DBInstance returns back to
+	// available.
+	if a.ko.Spec.AvailabilityZone == nil &&
+		b.ko.Spec.AvailabilityZone != nil {
+		a.ko.Spec.AvailabilityZone = b.ko.Spec.AvailabilityZone
+	}
+	if a.ko.Spec.BackupTarget == nil &&
+		b.ko.Spec.BackupTarget != nil &&
+		*b.ko.Spec.BackupTarget == ServiceDefaultBackupTarget {
+		a.ko.Spec.BackupTarget = b.ko.Spec.BackupTarget
+	}
+	if a.ko.Spec.NetworkType == nil &&
+		b.ko.Spec.NetworkType != nil &&
+		*b.ko.Spec.NetworkType == ServiceDefaultNetworkType {
+		a.ko.Spec.NetworkType = b.ko.Spec.NetworkType
+	}
+	if a.ko.Spec.PerformanceInsightsRetentionPeriod == nil &&
+		b.ko.Spec.PerformanceInsightsRetentionPeriod != nil &&
+		*b.ko.Spec.PerformanceInsightsRetentionPeriod == ServiceDefaultInsightsRetentionPeriod {
+		a.ko.Spec.PerformanceInsightsRetentionPeriod = b.ko.Spec.PerformanceInsightsRetentionPeriod
+	}
+	if a.ko.Spec.PerformanceInsightsKMSKeyID == nil &&
+		b.ko.Spec.PerformanceInsightsKMSKeyID != nil {
+		a.ko.Spec.PerformanceInsightsKMSKeyID = b.ko.Spec.PerformanceInsightsKMSKeyID
+	}
+
+	// RDS will choose preferred engine minor version if only
+	// engine major version is provided and controler should not
+	// treat them as different, such as spec has 14, status has 14.1
+	// controller should treat them as same
+	reconcileEngineVersion(a, b)
     compareTags(delta, a, b)

pkg/resource/db_instance/sdk.go

@@ -25,6 +25,7 @@
 from e2e import condition
 from e2e import db_instance
 from e2e.fixtures import k8s_secret
+from e2e import tag
 
 RESOURCE_PLURAL = 'dbinstances'
 
@@ -180,7 +181,7 @@ def test_crud_postgres14_t3_micro(
         expect_tags = [
             {"Key": "environment", "Value": "dev"}
         ]
-        latest_tags = db_instance.get_tags(arn)
+        latest_tags = tag.clean(db_instance.get_tags(arn))
         assert expect_tags == latest_tags
 
         # OK, now let's update the tag set and check that the tags are
@@ -197,7 +198,7 @@ def test_crud_postgres14_t3_micro(
         k8s.patch_custom_resource(ref, updates)
         time.sleep(MODIFY_WAIT_AFTER_SECONDS)
 
-        latest_tags = db_instance.get_tags(arn)
+        latest_tags = tag.clean(db_instance.get_tags(arn))
         after_update_expected_tags = [
             {
                 "Key": "environment",
@@ -259,4 +260,4 @@ def test_enable_pi_postgres14_t3_micro(
 
         # TODO: Ensure that the server side defaults
         # (PerformanceInsightsRetentionPeriod and PerformanceInsightsKMSKeyID)
-        # are also persisted back into the spec. This currently does not work
+        # are also persisted back into the spec. This currently does not work