-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
[SFN] SNS Optimised Integration: Automatic Stringification of Message Arguments #10165
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
Conversation
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.
minor nit, otherwise LGTM
Really appreciate the ultra fast turnaround time on this 💨
if "Message" in normalised_parameters: | ||
message = normalised_parameters["Message"] | ||
if not isinstance(message, str): | ||
normalised_parameters["Message"] = to_json_str(message) |
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.
if "Message" in normalised_parameters: | |
message = normalised_parameters["Message"] | |
if not isinstance(message, str): | |
normalised_parameters["Message"] = to_json_str(message) | |
if message := normalized_parameters.get("Message"): | |
if not isinstance(message, str): | |
normalized_parameters["Message"] = to_json_str(message) |
bit of a nit (walrus operator)
also we generally try to use American English for consistency
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.
Thank you, Dominik! The walrus operator really improves readability in this case. I changed it slightly to ensure it wouldn't add missing 'Message' parameters (in case this is not given by the program).
About the British spelling, I realise it's a problem for this provider, I'll open a PR to standardise all of it!
"Message": { | ||
"arg2": "World", | ||
"arg1": "Hello", | ||
"TaskToken": "<task_token:1>" |
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.
🥳 nice
…f Message Arguments (#10165)
Motivation
Currently, the StepFunctions v2 interpreter lacks support for automatic stringification of message arguments in the SNS Optimised Integration. This limitation can lead to unintended behaviour in publish calls. This PR addresses such lacking support and adds relevant validated snapshot tests for SNS integrations, including a callback scenario.
Changes