fix(metricalarm): sync tags via TagResource/UntagResource on update - #76
fix(metricalarm): sync tags via TagResource/UntagResource on update#76mahasiva-amazon wants to merge 2 commits into
Conversation
DescribeAlarms does not return tags. Add a sdk_read_many_post_set_output hook that calls ListTagsForResource after each DescribeAlarms response so spec.Tags always reflects the current AWS state, eliminating spurious reconcile loops when tags are present. Mark AlarmArn as is_arn so the codegen maps it to status.ackResourceMetadata.arn — required for the tags hook and for proper ACK ARN-based cross-resource references. Fix: test/e2e/metric_alarm.py uses ResourceARN (not ResourceName) for list_tags_for_resource, which is the correct CloudWatch parameter. Add test_tag_sync to TestMetricAlarm to exercise the full tag read-back cycle, and add the metric_alarm_with_tags resource fixture.
PutMetricAlarm silently ignores the Tags field when updating an existing alarm. This commit implements a syncTags hook that is called from sdkUpdate whenever Spec.Tags differs between the desired and observed states. The hook calls TagResource to add/update tags and UntagResource to remove tags that are no longer in the spec. A short-circuit is also added so that a tags-only delta does not trigger an unnecessary PutMetricAlarm call (DifferentExcept guard). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mahasiva-amazon The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @mahasiva-amazon. Thanks for your PR. I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Fixes aws-controllers-k8s/community#2985
PutMetricAlarmsilently ignores theTagsfield when updating an existing alarm. As a result, tag changes made to a MetricAlarm CR were never propagated to AWS.Additionally,
DescribeAlarmsdoes not return tags, so tags must be fetched separately viaListTagsForResourceon every reconcile.Solution
This PR fixes both issues:
Read path (
sdk_read_many_post_set_outputhook, committed in prior commit): CallsListTagsForResourceafter everyDescribeAlarmsto populateSpec.Tagsin the observed state, enabling accurate delta detection.Write path (this commit): Adds a
syncTagsfunction inhooks.gothat is called fromsdkUpdatewheneverSpec.Tagsdiffers. It callsTagResourceto add/update tags andUntagResourceto remove tags no longer in the spec.Short-circuit: A
DifferentExcept("Spec.Tags")guard prevents an unnecessaryPutMetricAlarmcall when the delta is tags-only.Files Changed
generator.yaml— addssdk_update_pre_build_requesthook for MetricAlarmtemplates/hooks/metricalarm/sdk_update_pre_build_request.go.tpl— new hook template with tag sync and short-circuit logicpkg/resource/metric_alarm/hooks.go— new file withsyncTagsimplementationpkg/resource/metric_alarm/sdk.go— regenerated with hook injected intosdkUpdateTest plan
env: testtag — alarm created in AWS with correct tagteam: platformtag — AWS alarm updated viaTagResourceteam: platformtag — AWS alarm updated viaUntagResourcePutMetricAlarmcallACK.ResourceSynced=TrueandReady=Trueafter each reconcilego build ./...🤖 Generated with Claude Code