Skip to content

feat: add CompareResourceForPreDelete and DeltaForPreDelete support#686

Open
sapphirew wants to merge 2 commits intoaws-controllers-k8s:mainfrom
sapphirew:pre-delete-sync
Open

feat: add CompareResourceForPreDelete and DeltaForPreDelete support#686
sapphirew wants to merge 2 commits intoaws-controllers-k8s:mainfrom
sapphirew:pre-delete-sync

Conversation

@sapphirew
Copy link
Copy Markdown
Contributor

@sapphirew sapphirew commented Apr 8, 2026 •
edited
Loading

Add pre-delete delta comparison support to the code generator. Controllers can opt specific fields into pre-delete comparison via compare.pre_delete_include: true in generator.yaml, so that fields like DeletionProtectionEnabled are synced before deletion even when they use compare.is_ignored for normal reconciliation.

Config changes:

  • Add PreDeleteInclude bool to CompareFieldConfig (field.go)
  • Add PreDeleteSyncConfig with CompareAll to ResourceConfig (resource.go)

Code generation:

  • Add CompareResourceForPreDelete function that emits comparison code
    only for opted-in fields (or all fields when compare_all is true)
  • Register GoCodeCompareForPreDelete template function (controller.go)
  • Add newResourceDeltaForPreDelete in delta.go.tpl
  • Add DeltaForPreDelete method on resourceDescriptor in descriptor.go.tpl

Tests:

  • Four tests covering no-opt-in (empty), explicit opt-in, compare_all,
    and empty-without-opt-in scenarios
  • Two new S3 test fixture generator YAMLs

runtime changes: aws-controllers-k8s/runtime#234

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ack-prow ack-prow Bot requested review from a-hilaly and jlbutler April 8, 2026 18:15
michaelhtm
michaelhtm reviewed Apr 8, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@michaelhtm michaelhtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sapphirew
left one comment below

Comment thread pkg/generate/code/compare.go Outdated
Comment on lines +992 to +997
// CompareResourceForPreDelete returns the Go code that traverses a set of two
// Resources, adding differences between the two Resources to an
// `ackcompare.Delta`. Unlike CompareResource, this function does NOT skip
// fields where compareConfig.IsIgnored is true. This is used for pre-delete
// sync to ensure fields like DeletionProtectionEnabled are included in the
// delta comparison.
Copy link
Copy Markdown
Member

@michaelhtm michaelhtm Apr 8, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we make this delta comparison opt-in? Only compute delta on specific fields defined in generator.yaml?
In dsql example, we could just compare DeletionProtection field only

Copy link
Copy Markdown
Contributor Author

@sapphirew sapphirew Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call, that would provide fine-control on specific fields for a controller.

The current delta comparison can be enabled by a flag like this instead:

resources:
  Cluster:
    pre_delete_sync:
      compare_all: true

@rhaowang
feat(codegen): Add opt-in CompareResourceForPreDelete and DeltaForPre…
b3f5d67 
…Delete support

Add pre-delete delta comparison support to the code generator. Controllers
can opt specific fields into pre-delete comparison via
compare.pre_delete_include: true in generator.yaml, so that fields like
DeletionProtectionEnabled are synced before deletion even when they use
compare.is_ignored for normal reconciliation.

Config changes:
- Add PreDeleteInclude bool to CompareFieldConfig (field.go)
- Add PreDeleteSyncConfig with CompareAll to ResourceConfig (resource.go)

Code generation:
- Add CompareResourceForPreDelete function that emits comparison code
  only for opted-in fields (or all fields when compare_all is true)
- Register GoCodeCompareForPreDelete template function (controller.go)
- Add newResourceDeltaForPreDelete in delta.go.tpl
- Add DeltaForPreDelete method on resourceDescriptor in descriptor.go.tpl

Tests:
- Four tests covering no-opt-in (empty), explicit opt-in, compare_all,
  and empty-without-opt-in scenarios
- Two new S3 test fixture generator YAMLs
@sapphirew
Copy link
Copy Markdown
Contributor Author

sapphirew commented Apr 8, 2026

/test s3-controller-test

@sapphirew
Copy link
Copy Markdown
Contributor Author

sapphirew commented Apr 8, 2026

/test lambda-controller-test

@sapphirew
Copy link
Copy Markdown
Contributor Author

sapphirew commented Apr 8, 2026

/retest

2 similar comments
@sapphirew
Copy link
Copy Markdown
Contributor Author

sapphirew commented Apr 9, 2026

/retest

@sapphirew
Copy link
Copy Markdown
Contributor Author

sapphirew commented Apr 9, 2026

/retest

sapphirew added a commit to sapphirew/ack-sns-controller that referenced this pull request Apr 17, 2026
@sapphirew @rhaowang
feat(topic): Add FifoThroughputScope attribute support
f907f96 
Add FifoThroughputScope to generator.yaml with is_attribute: true and
regenerate controller code. Fix the custom update hook in hooks.go by
adding FIFOThroughputScope to settableAttributeNames and the
newSetAttributesRequestPayload switch statement.

Add e2e test coverage: create assertions for FifoThroughputScope and
an update test verifying changes via SetTopicAttributes.

ArchivePolicy support is deferred until the pre-delete-sync feature
lands (aws-controllers-k8s/code-generator#686), since SNS rejects
DeleteTopic on topics with an active ArchivePolicy and the controller
cannot sync spec updates after a deletion timestamp is set.

Closes: aws-controllers-k8s/community#2840
sapphirew added a commit to sapphirew/ack-sns-controller that referenced this pull request Apr 17, 2026
@sapphirew @rhaowang
feat(topic): Add FifoThroughputScope attribute support
e79bb44 
Add FifoThroughputScope to generator.yaml with is_attribute: true and
regenerate controller code. Fix the custom update hook in hooks.go by
adding FIFOThroughputScope to settableAttributeNames and the
newSetAttributesRequestPayload switch statement.

Add e2e test coverage: create assertions for FifoThroughputScope and
an update test verifying changes via SetTopicAttributes.

ArchivePolicy support is deferred until the pre-delete-sync feature
lands (aws-controllers-k8s/code-generator#686), since SNS rejects
DeleteTopic on topics with an active ArchivePolicy and the controller
cannot sync spec updates after a deletion timestamp is set.

Closes: aws-controllers-k8s/community#2840
sapphirew added a commit to sapphirew/ack-sns-controller that referenced this pull request Apr 17, 2026
@sapphirew @rhaowang
feat(topic): Add FifoThroughputScope attribute support
97a54ce 
Add FifoThroughputScope to generator.yaml with is_attribute: true and
regenerate controller code. Fix the custom update hook in hooks.go by
adding FIFOThroughputScope to settableAttributeNames and the
newSetAttributesRequestPayload switch statement.

Add e2e test coverage: create assertions for FifoThroughputScope and
an update test verifying changes via SetTopicAttributes.

ArchivePolicy support is deferred until the pre-delete-sync feature
lands (aws-controllers-k8s/code-generator#686), since SNS rejects
DeleteTopic on topics with an active ArchivePolicy and the controller
cannot sync spec updates after a deletion timestamp is set.

Closes: aws-controllers-k8s/community#2840
sapphirew added a commit to sapphirew/ack-sns-controller that referenced this pull request Apr 17, 2026
@sapphirew @rhaowang
feat(topic): Add FifoThroughputScope attribute support
e515353 
Add FifoThroughputScope to generator.yaml with is_attribute: true and
regenerate controller code. Fix the custom update hook in hooks.go by
adding FIFOThroughputScope to settableAttributeNames and the
newSetAttributesRequestPayload switch statement.

Add e2e test coverage: create assertions for FifoThroughputScope and
an update test verifying changes via SetTopicAttributes.

ArchivePolicy support is deferred until the pre-delete-sync feature
lands (aws-controllers-k8s/code-generator#686), since SNS rejects
DeleteTopic on topics with an active ArchivePolicy and the controller
cannot sync spec updates after a deletion timestamp is set.

Closes: aws-controllers-k8s/community#2840
@sapphirew sapphirew requested a review from michaelhtm April 20, 2026 18:25
@sapphirew
Copy link
Copy Markdown
Contributor Author

sapphirew commented Apr 20, 2026

/retest

knottnt
knottnt reviewed Apr 21, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@knottnt knottnt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sapphirew left a few comments. Also, it would be helpful to see a WIP PR showing off these changes applied to a service controller. It makes it a bit easier to understand how the generated code will actually work.

Comment thread pkg/generate/code/compare.go Outdated
Comment thread pkg/generate/code/compare.go Outdated
Comment thread templates/pkg/resource/descriptor.go.tpl Outdated
knottnt
knottnt reviewed Apr 21, 2026
View reviewed changes
Comment thread pkg/generate/code/compare.go
Comment thread templates/pkg/resource/descriptor.go.tpl
Comment thread CODE_GENERATOR_FIX_BLOB_UNION.md Outdated
@sapphirew sapphirew force-pushed the pre-delete-sync branch 3 times, most recently from dce77f2 to 67890b7 Compare April 22, 2026 00:16
michaelhtm
michaelhtm reviewed Apr 22, 2026
View reviewed changes
Comment thread pkg/generate/code/compare_test.go
Comment on lines +704 to +711
expected := `
if ackcompare.HasNilDifference(a.ko.Spec.ACL, b.ko.Spec.ACL) {
delta.Add("Spec.ACL", a.ko.Spec.ACL, b.ko.Spec.ACL)
} else if a.ko.Spec.ACL != nil && b.ko.Spec.ACL != nil {
if *a.ko.Spec.ACL != *b.ko.Spec.ACL {
delta.Add("Spec.ACL", a.ko.Spec.ACL, b.ko.Spec.ACL)
}
}
Copy link
Copy Markdown
Member

@michaelhtm michaelhtm Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add test cases for nested fields?

Copy link
Copy Markdown
Contributor Author

@sapphirew sapphirew Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, added

Comment thread pkg/generate/code/compare.go
// The generated code is a series of simple field assignments for each field
// that has `compare.pre_delete_include: true` (or all fields when
// `pre_delete_sync.compare_all: true`).
func MergeResourceForPreDelete(
Copy link
Copy Markdown
Member

@michaelhtm michaelhtm Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a unit test for this one?

Copy link
Copy Markdown
Contributor Author

@sapphirew sapphirew Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, added

@sapphirew
feat(codegen): Return merged resource from DeltaForPreDelete
0d67d28 
Update DeltaForPreDelete to return both a delta and a merged resource.
The merged resource is a deep copy of observed (b) with only the
pre-delete sync configured fields overwritten from desired (a). This
ensures rm.Update only changes the fields that DeltaForPreDelete
detected as different while all other fields retain their current AWS
values.

Refactor comparison logic:
- Extract compareResourceFields shared function with fieldFilter
  callback to eliminate duplicated comparison code between
  CompareResource and CompareResourceForPreDelete
- Extract sortedSpecFieldNames helper to deduplicate sorted field
  iteration across CompareResource, CompareResourceForPreDelete,
  and MergeResourceForPreDelete
- Add MergeResourceForPreDelete and GoCodeMergeForPreDelete template
  function for generating field assignments
- Simplify descriptor.go.tpl DeltaForPreDelete by removing redundant
  nil check
@michaelhtm
Copy link
Copy Markdown
Member

michaelhtm commented Apr 22, 2026

/test lambda-controller-test

@sapphirew
Copy link
Copy Markdown
Contributor Author

sapphirew commented Apr 23, 2026

/retest

@ack-prow
Copy link
Copy Markdown

ack-prow Bot commented Apr 24, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knottnt, sapphirew

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ack-prow ack-prow Bot added the approved label Apr 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelhtm michaelhtm michaelhtm left review comments

@knottnt knottnt knottnt approved these changes

@a-hilaly a-hilaly Awaiting requested review from a-hilaly

@jlbutler jlbutler Awaiting requested review from jlbutler

Assignees

No one assigned

Labels

approved

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sapphirew @michaelhtm @knottnt @rhaowang