From 6b22f03957eeee069b69761ecbe1dcde38be4912 Mon Sep 17 00:00:00 2001 From: michaelhtm <98621731+michaelhtm@users.noreply.github.com> Date: Tue, 8 Apr 2025 09:13:08 -0700 Subject: [PATCH 1/2] chore: Ensure controller understands both ENABLE & ENABLED contributorInsights --- pkg/resource/table/hooks.go | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/pkg/resource/table/hooks.go b/pkg/resource/table/hooks.go index 41f9471..f12b762 100644 --- a/pkg/resource/table/hooks.go +++ b/pkg/resource/table/hooks.go @@ -774,7 +774,18 @@ func (rm *resourceManager) setContributorInsights( if err != nil { return err } - + + // This portion is needed if we want to have a smooth delta comparison + // If the name ends in ED (ENABLED, DISABLED) just assign the value + endsWithED := false + if ko.Spec.ContributorInsights != nil { + endsWithED = strings.HasSuffix(*ko.Spec.TableName, "ED") + } + if endsWithED { + ko.Spec.ContributorInsights = aws.String(string(resp.ContributorInsightsStatus)) + return nil + } + // if not do the conversion switch resp.ContributorInsightsStatus { case svcsdktypes.ContributorInsightsStatusEnabled: ko.Spec.ContributorInsights = aws.String(string(svcsdktypes.ContributorInsightsActionEnable)) @@ -797,9 +808,18 @@ func (rm *resourceManager) updateContributorInsights( defer func() { exit(err) }() + insight := svcsdktypes.ContributorInsightsActionDisable if r.ko.Spec.ContributorInsights != nil { - insight = svcsdktypes.ContributorInsightsAction(*r.ko.Spec.ContributorInsights) + // We will allow users to provide values ENABLE, ENABLED, DISABLE, DISABLED + switch *r.ko.Spec.ContributorInsights { + case string(svcsdktypes.ContributorInsightsActionEnable), string(svcsdktypes.ContributorInsightsStatusEnabled): + insight = svcsdktypes.ContributorInsightsActionEnable + case string(svcsdktypes.ContributorInsightsActionDisable), string(svcsdktypes.ContributorInsightsStatusDisabled): + insight = svcsdktypes.ContributorInsightsActionDisable + default: + return fmt.Errorf("invalid ContributorInsights value: %s", *r.ko.Spec.ContributorInsights) + } } _, err = rm.sdkapi.UpdateContributorInsights( From edff42d9ad5f63244ea03bf7805f96dc77b15e81 Mon Sep 17 00:00:00 2001 From: michaelhtm <98621731+michaelhtm@users.noreply.github.com> Date: Tue, 8 Apr 2025 10:04:50 -0700 Subject: [PATCH 2/2] fix: increate test wait time --- apis/v1alpha1/ack-generate-metadata.yaml | 8 +-- apis/v1alpha1/generator.yaml | 2 + generator.yaml | 2 + pkg/resource/table/delta.go | 7 --- pkg/resource/table/hooks.go | 63 +++++++++++++----------- test/e2e/resources/table_insights.yaml | 2 +- test/e2e/tests/test_table.py | 4 +- 7 files changed, 45 insertions(+), 43 deletions(-) diff --git a/apis/v1alpha1/ack-generate-metadata.yaml b/apis/v1alpha1/ack-generate-metadata.yaml index 72228a1..32d440a 100755 --- a/apis/v1alpha1/ack-generate-metadata.yaml +++ b/apis/v1alpha1/ack-generate-metadata.yaml @@ -1,13 +1,13 @@ ack_generate_info: - build_date: "2025-04-01T02:35:33Z" - build_hash: 980cb1e4734f673d16101cf55206b84ca639ec01 + build_date: "2025-04-08T22:25:51Z" + build_hash: 0909e7f0adb8ffe4120a8c20d5d58b991f2539e9 go_version: go1.24.1 - version: v0.44.0 + version: v0.44.0-3-g0909e7f api_directory_checksum: 2f94829f12ad90f9c36c48823459c161c1670093 api_version: v1alpha1 aws_sdk_go_version: v1.32.6 generator_config_info: - file_checksum: e7e79c3c7c21273967ca24947fda0f26b344c8f0 + file_checksum: cc3489b53a45170d339a4de0d7d7ec0aa788955e original_file_name: generator.yaml last_modification: reason: API generation diff --git a/apis/v1alpha1/generator.yaml b/apis/v1alpha1/generator.yaml index 3dd2453..47dc75c 100644 --- a/apis/v1alpha1/generator.yaml +++ b/apis/v1alpha1/generator.yaml @@ -74,6 +74,8 @@ resources: from: operation: UpdateContributorInsights path: ContributorInsightsAction + compare: + is_ignored: true exceptions: errors: 404: diff --git a/generator.yaml b/generator.yaml index 3dd2453..47dc75c 100644 --- a/generator.yaml +++ b/generator.yaml @@ -74,6 +74,8 @@ resources: from: operation: UpdateContributorInsights path: ContributorInsightsAction + compare: + is_ignored: true exceptions: errors: 404: diff --git a/pkg/resource/table/delta.go b/pkg/resource/table/delta.go index 954881e..520d86a 100644 --- a/pkg/resource/table/delta.go +++ b/pkg/resource/table/delta.go @@ -62,13 +62,6 @@ func newResourceDelta( } } } - if ackcompare.HasNilDifference(a.ko.Spec.ContributorInsights, b.ko.Spec.ContributorInsights) { - delta.Add("Spec.ContributorInsights", a.ko.Spec.ContributorInsights, b.ko.Spec.ContributorInsights) - } else if a.ko.Spec.ContributorInsights != nil && b.ko.Spec.ContributorInsights != nil { - if *a.ko.Spec.ContributorInsights != *b.ko.Spec.ContributorInsights { - delta.Add("Spec.ContributorInsights", a.ko.Spec.ContributorInsights, b.ko.Spec.ContributorInsights) - } - } if ackcompare.HasNilDifference(a.ko.Spec.DeletionProtectionEnabled, b.ko.Spec.DeletionProtectionEnabled) { delta.Add("Spec.DeletionProtectionEnabled", a.ko.Spec.DeletionProtectionEnabled, b.ko.Spec.DeletionProtectionEnabled) } else if a.ko.Spec.DeletionProtectionEnabled != nil && b.ko.Spec.DeletionProtectionEnabled != nil { diff --git a/pkg/resource/table/hooks.go b/pkg/resource/table/hooks.go index f12b762..df87e72 100644 --- a/pkg/resource/table/hooks.go +++ b/pkg/resource/table/hooks.go @@ -615,8 +615,16 @@ func customPreCompare( // This will ensure controller does not act on this field // if user is unaware of it. if a.ko.Spec.ContributorInsights == nil { - a.ko.Spec.ContributorInsights = b.ko.Spec.ContributorInsights + return } + // latestInsight will always be either ENABLED or DISABLED, since we requeue at readOne if its not + // either + desiredInsight, _ := ensureContibutorInsight(a) + latestInsight, _ := ensureContibutorInsight(b) + if desiredInsight != latestInsight { + delta.Add("Spec.ContributorInsights", a.ko.Spec.ContributorInsights, b.ko.Spec.ContributorInsights) + } + } // equalAttributeDefinitions return whether two AttributeDefinition arrays are equal or not. @@ -774,41 +782,18 @@ func (rm *resourceManager) setContributorInsights( if err != nil { return err } - - // This portion is needed if we want to have a smooth delta comparison - // If the name ends in ED (ENABLED, DISABLED) just assign the value - endsWithED := false + + // Only manage ContributorInsights if it is defined if ko.Spec.ContributorInsights != nil { - endsWithED = strings.HasSuffix(*ko.Spec.TableName, "ED") - } - if endsWithED { ko.Spec.ContributorInsights = aws.String(string(resp.ContributorInsightsStatus)) - return nil - } - // if not do the conversion - switch resp.ContributorInsightsStatus { - case svcsdktypes.ContributorInsightsStatusEnabled: - ko.Spec.ContributorInsights = aws.String(string(svcsdktypes.ContributorInsightsActionEnable)) - case svcsdktypes.ContributorInsightsStatusDisabled: - ko.Spec.ContributorInsights = aws.String(string(svcsdktypes.ContributorInsightsActionDisable)) - default: - ko.Spec.ContributorInsights = aws.String(string(resp.ContributorInsightsStatus)) - } return nil } -func (rm *resourceManager) updateContributorInsights( - ctx context.Context, - r *resource, -) (err error) { - rlog := ackrtlog.FromContext(ctx) - exit := rlog.Trace("rm.updateCloudformationInsights") - defer func() { - exit(err) - }() - +// ensureContibutorInsight ensures the controller understands ENABLE and ENABLED are the same. +// We choose to return ContributorInsightsAction because that is the enum used to update. +func ensureContibutorInsight(r *resource) (svcsdktypes.ContributorInsightsAction, error) { insight := svcsdktypes.ContributorInsightsActionDisable if r.ko.Spec.ContributorInsights != nil { // We will allow users to provide values ENABLE, ENABLED, DISABLE, DISABLED @@ -818,10 +803,28 @@ func (rm *resourceManager) updateContributorInsights( case string(svcsdktypes.ContributorInsightsActionDisable), string(svcsdktypes.ContributorInsightsStatusDisabled): insight = svcsdktypes.ContributorInsightsActionDisable default: - return fmt.Errorf("invalid ContributorInsights value: %s", *r.ko.Spec.ContributorInsights) + return "", fmt.Errorf("invalid ContributorInsights value: %s", *r.ko.Spec.ContributorInsights) } } + return insight, nil +} + +func (rm *resourceManager) updateContributorInsights( + ctx context.Context, + r *resource, +) (err error) { + rlog := ackrtlog.FromContext(ctx) + exit := rlog.Trace("rm.updateCloudformationInsights") + defer func() { + exit(err) + }() + + insight, err := ensureContibutorInsight(r) + if err != nil { + return fmt.Errorf("failed preparing contributorInsight: %v", err) + } + _, err = rm.sdkapi.UpdateContributorInsights( ctx, &svcsdk.UpdateContributorInsightsInput{ diff --git a/test/e2e/resources/table_insights.yaml b/test/e2e/resources/table_insights.yaml index 9ee4a1e..37e0664 100644 --- a/test/e2e/resources/table_insights.yaml +++ b/test/e2e/resources/table_insights.yaml @@ -7,7 +7,7 @@ spec: tableName: $TABLE_NAME billingMode: PAY_PER_REQUEST tableClass: STANDARD - contributorInsights: ENABLE + contributorInsights: ENABLED attributeDefinitions: - attributeName: Bill attributeType: S diff --git a/test/e2e/tests/test_table.py b/test/e2e/tests/test_table.py index 49226e5..cad55f6 100644 --- a/test/e2e/tests/test_table.py +++ b/test/e2e/tests/test_table.py @@ -279,6 +279,8 @@ def test_enable_ttl(self, table_lsi): # Patch k8s resource k8s.patch_custom_resource(ref, updates) + time.sleep(MODIFY_WAIT_AFTER_SECONDS) + table.wait_until( table_name, table.ttl_on_attribute_matches("ForumName"), @@ -480,7 +482,7 @@ def test_update_insights(self, table_insights): # Check DynamoDB Table exists assert self.table_exists(table_name) - assert cr['spec']['contributorInsights'] == "ENABLE" + assert cr['spec']['contributorInsights'] == "ENABLED" assert self.table_insight_status(table_name, "ENABLED") # Set provisionedThroughput