Skip to content

Commit

Permalink
correct delta_pre_compare for DBInstance
Browse files Browse the repository at this point in the history
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 <jaypipes@gmail.com>
  • Loading branch information
jaypipes committed Aug 13, 2022
1 parent f0864ae commit 647df8e
Show file tree
Hide file tree
Showing 8 changed files with 83 additions and 68 deletions.
4 changes: 2 additions & 2 deletions apis/v1alpha1/ack-generate-metadata.yaml
Original file line number Diff line number Diff line change
@@ -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
4 changes: 2 additions & 2 deletions apis/v1alpha1/generator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions generator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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
Expand Down
38 changes: 37 additions & 1 deletion pkg/resource/db_instance/delta.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

39 changes: 0 additions & 39 deletions pkg/resource/db_instance/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 0 additions & 7 deletions pkg/resource/db_instance/sdk.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

48 changes: 36 additions & 12 deletions templates/hooks/db_instance/delta_pre_compare.go.tpl
Original file line number Diff line number Diff line change
@@ -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)
7 changes: 4 additions & 3 deletions test/e2e/tests/test_db_instance.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand Down Expand Up @@ -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
Expand All @@ -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",
Expand Down Expand Up @@ -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

0 comments on commit 647df8e

Please sign in to comment.