-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Sns topic update functionality #12777
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Sns topic update functionality #12777
Conversation
All contributors have signed the CLA ✍️ ✅ |
recheck |
1 similar comment
recheck |
I have read the CLA Document and I hereby sign the CLA. |
recheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for posting the updates to this PR. I have left some follow ups on the previous PR and added some comments here. In general I am ok with this PR. I think we have captured a good part of the update implementation for SNS topics, including using CFn to manage the subscriptions.
I would be interested to capture the behaviour you had previously in a follow up where the subscriptions are created outside of CFn, however I think this use case is less common and therefore not as high a priority as the changes you've added here.
Thanks very much!
(I have requested changes again as I still have a couple of outstanding comments that have not been addressed. These are only minor though, I just would like your take on them before I approve).
localstack-core/localstack/services/sns/resource_providers/aws_sns_topic.py
Show resolved
Hide resolved
localstack-core/localstack/services/sns/resource_providers/aws_sns_topic.py
Show resolved
Hide resolved
tag_dict = {tag["Key"]: tag["Value"] for tag in initial_tags["Tags"]} | ||
assert tag_dict["Environment"] == "test" | ||
assert tag_dict["Project"] == "localstack" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tag_dict = {tag["Key"]: tag["Value"] for tag in initial_tags["Tags"]} | |
assert tag_dict["Environment"] == "test" | |
assert tag_dict["Project"] == "localstack" | |
snapshot.match("initial-tags", initial_tags) |
We may need to add specific non-supported tags to the skip_snapshot_verify
decorator at the top of the test function, as CFn adds its own tags to resources and we don't support that yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my initial plan. I couldn't make it work though because of how the Tags are formatted. I couldn't find a way to skip the verification based on the Key. Also, AWS creates the tags in a random order and this also affects the validation. My idea was to temporarily check the tags manually until we find a proper solution (which might be already there but I missed it). I checked other examples but the tags there were formatted differently e.g. in Lambda snapshots tags are all in a single dictionary (not a list of dicts like bellow).
E.g.:
"Tags": [
{
"Key": "Project",
"Value": "localstack"
},
{
"Key": "aws:cloudformation:stack-name",
"Value": "replaced-value"
},
{
"Key": "Environment",
"Value": "test"
},
{
"Key": "aws:cloudformation:logical-id",
"Value": "TestTopic"
},
{
"Key": "aws:cloudformation:stack-id",
"Value": "replaced-value"
}
],
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's fair enough for now. Thanks for explaining.
updated_tag_dict = {tag["Key"]: tag["Value"] for tag in updated_tags["Tags"]} | ||
assert updated_tag_dict["Environment"] == "production" | ||
assert updated_tag_dict["Project"] == "backend" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated_tag_dict = {tag["Key"]: tag["Value"] for tag in updated_tags["Tags"]} | |
assert updated_tag_dict["Environment"] == "production" | |
assert updated_tag_dict["Project"] == "backend" | |
snapshot.match("updated-tags", updated_tags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related PR: #12768
Motivation
This PR implements basic update support for the AWS::SNS::Topic CloudFormation resource, addressing the core requirement to enable updates for DisplayName, TopicName, and Tags properties. The implementation follows AWS CloudFormation's update semantics, including resource replacement when immutable properties change.
Changes
Enhanced Resource Provider (
aws_sns_topic.py
)Implemented complete
update()
method with support for:DisplayName
attribute changesTopicName
changes (recreates topic with new name)Key features:
Comprehensive Test Coverage (
test_sns.py
)Created
test_sns_topic_update_attributes()
with in-place updates (no resource replacement):DisplayName
attribute changes are applied correctlyCreated
test_sns_topic_update_name()
with resource replacement:Added new entries to the Snapshot File (
test_sns.snapshot.json
)Testing
TODO
Test against AWS and find out whytest_sns_topic_update_name
test fails on AWS but not locally.Proposed Next Steps
1: Complete Core Properties
2: Advanced Update Capabilities
3: Robustness & Edge Cases
4: Testing