Sns:v2 platform endpoint operations #13327
Open
+1,437
−26
Conversation
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
Test Results - Preflight, Unit22 278 tests - 98 20 526 ✅ - 100 15m 54s ⏱️ -4s Results for commit 3259419. ± Comparison against base commit f829e69. This pull request removes 170 and adds 72 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Test Results - Alternative Providers198 tests - 1 240 78 ✅ - 707 43s ⏱️ - 39m 36s Results for commit 3259419. ± Comparison against base commit f829e69. This pull request removes 1262 and adds 22 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
baermat
commented
Oct 31, 2025
Comment on lines
151
to
154
| class PlatformEndpoint(TypedDict, total=False): | ||
| PlatformEndpointArn: str | ||
| Attributes: dict[str, str] | ||
| PlatformApplicationArn: str |
There was a problem hiding this comment.
This type exists in the Api as well, but without the PlatformApplicationArn. Having a reference back to the owning object makes delete easier so we don't have to iterate over all applications, however it might not be worth losing the advantage of sticking exactly to the spec. Wdyt?
There was a problem hiding this comment.
Very good point! I believe @giograno is starting to establish a pattern here, where we use a dataclass to wrap the existing spec type, and add more data to it.
See https://github.com/localstack/localstack-pro/pull/5397
Something like:
from localstack.aws.api.sns import PlatformApplication
@dataclasses.dataclass
class PlatformApplicationDetails:
platform_application: PlatformApplication
platform_application_arn: strI hope it's okay, I'll go ahead and push those to get the PR in this week 👍
Comment on lines
725
to
730
| Endpoints=[ | ||
| { | ||
| "EndpointArn": endpoint_arn, | ||
| "Attributes": store.platform_endpoints.get(endpoint_arn, {}).get("Attributes"), | ||
| } | ||
| for endpoint_arn in page |
There was a problem hiding this comment.
this is slightly more complex than it needs to be due to the mentioned deviation of the type. A good example of the trade off I mentioned before
baermat
added
semver: minor
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 59m 8s ⏱️ - 1h 4m 12s Results for commit 3259419. ± Comparison against base commit f829e69. This pull request removes 1990 and adds 22 tests. Note that renamed tests count towards both.This pull request removes 224 skipped tests and adds 10 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 27m 29s ⏱️ - 1h 12m 42s Results for commit 3259419. ± Comparison against base commit f829e69. This pull request removes 2340 and adds 22 tests. Note that renamed tests count towards both.This pull request removes 387 skipped tests and adds 10 skipped tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 adds the platform application crud operations as well as the operations for the attributes, alongside tests including those for all special cases mentioned in the docs for these operations that I could find.
Currently failing test will be changed to not use list (so old remnants from other tests don't interfere)
Changes
Tests
Related
Closes PNX-86