CloudWatch V2: Move tagging functionality into separate methods to enable their overriding in pro version#13748
Open
CloudWatch V2: Move tagging functionality into separate methods to enable their overriding in pro version#13748
Conversation
Test Results - Alternative Providers186 tests - 1 087 39 ✅ - 687 2m 30s ⏱️ - 28m 36s For more details on these failures, see this check. Results for commit b187c4f. ± Comparison against base commit 2046dff. This pull request removes 1093 and adds 6 tests. Note that renamed tests count towards both.This pull request removes 406 skipped tests and adds 5 skipped tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Test Results (amd64) - Integration, Bootstrap 5 files ± 0 5 suites ±0 1h 5m 44s ⏱️ - 1h 30m 16s For more details on these failures, see this check. Results for commit b187c4f. ± Comparison against base commit 2046dff. This pull request removes 4346 and adds 6 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
LocalStack Community integration with Pro 2 files 2 suites 46m 6s ⏱️ For more details on these failures, see this check. Results for commit b187c4f. ♻️ This comment has been updated with latest results. |
aidehn
reviewed
Feb 13, 2026
| alarm_arn = alarm["AlarmArn"] | ||
|
|
||
| list_tags_for_resource = aws_cloudwatch_client.list_tags_for_resource(ResourceARN=alarm_arn) | ||
| snapshot.match("list_tags_for_resource_empty ", list_tags_for_resource) |
Contributor
There was a problem hiding this comment.
Is list tags for resource meant to return empty tags here? Based on the snapshot name I assume it should be empty but from the test logic I would have thought it'd have {"Key": "Environment", "Value": "Production"} as it's value (It might just be missing an additional put request here like how the below test does it)
| } | ||
| } | ||
| }, | ||
| "tests/aws/services/cloudwatch/test_cloudwatch.py::TestCloudwatch::test_tagging_metric_alarm[query]": { |
Contributor
There was a problem hiding this comment.
noticed that the test_tagging_metric_alarm test has been parametrized here but but doesn't have parametrization on the test itself, might be leftover?
Contributor
Author
There was a problem hiding this comment.
Yeah, this is a bit misleading. The parametrization is done in the aws_cloudwatch_client fixture
| } | ||
| } | ||
| }, | ||
| "tests/aws/services/cloudwatch/test_cloudwatch.py::TestCloudwatch::test_tagging_composite_alarm[query]": { |
Contributor
There was a problem hiding this comment.
Same question on parametrization here!
| self.region = region | ||
| self.alarm = alarm | ||
| # Tags are already stored as part of Tagging Service or RGTA plugin | ||
| self.alarm.pop("Tags") |
Contributor
There was a problem hiding this comment.
Based on the MetricAlarm model in CloudWatch I don't think it contains a Tags field which will raise here (same with the CompositeAlarm model) but it means we can likely remove this code and the code below 👍🏼
pinzon
reviewed
Feb 13, 2026
Comment on lines
+411
to
+435
| def _get_resource_tags( | ||
| self, resource_arn: AmazonResourceName, account_id: str, region: str | ||
| ) -> TagList: | ||
| store = self.get_store(account_id, region) | ||
| tags = store.TAGS.list_tags_for_resource(resource_arn).get("Tags", []) | ||
| return tags | ||
|
|
||
| def _set_resource_tags( | ||
| self, resource_arn: AmazonResourceName, account_id: str, region: str, tags: TagList | ||
| ) -> None: | ||
| store = self.get_store(account_id, region) | ||
| store.TAGS.tag_resource(arn=resource_arn, tags=tags) | ||
|
|
||
| def _remove_resource_tags( | ||
| self, resource_arn: AmazonResourceName, account_id: str, region: str, tag_keys: TagKeyList | ||
| ) -> None: | ||
| store = self.get_store(account_id, region) | ||
| store.TAGS.untag_resource(resource_arn, tag_keys) | ||
|
|
||
| def _remove_all_resource_tags( | ||
| self, resource_arn: AmazonResourceName, account_id: str, region: str | ||
| ): | ||
| store = self.get_store(account_id, region) | ||
| store.TAGS.del_resource(resource_arn) | ||
|
|
Member
There was a problem hiding this comment.
It could be worth it to look into the implementations of the Strategy pattern that we use in the ECS and IAM services. Where we:
- Write a simple entity that handles the simple functionality
- Instantiate that entity in the provider and make the provider use it in the operations
- In pro we define a different entity with more advance functionality and the pro plugin puts it in the provider.
IMO this makes the substitution of the method implementation more transparent to the maintainer.
Here is a sample
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
This PR moves tagging logic to separate methods on the CloudWatch V2 Provider class to enable their overriding in pro version and adds tagging logic to methods which create metric and composite alarms.
The separate methods will be overridden in the Pro version, where CloudWatch V2 will be integrated with the new ResourceGroupsTagging plugin. This integration will enable CloudWatch resources to be tagged and allow tags to be retrieved through the Resource Groups Tagging service.
Changes
Tests
Added integration tests to check tagging when resource is being created
Related
FLC-280