-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Fix Event Bridge input transformation of booleans #13236
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
Fix Event Bridge input transformation of booleans #13236
Conversation
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 58m 13s ⏱️ - 1h 0m 5s Results for commit b96ac19. ± Comparison against base commit 9af2464. This pull request removes 2156 and adds 2 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
91ac4a2 to
f108cbf
Compare
f108cbf to
1198117
Compare
| "call": 1.27, | ||
| "teardown": 1.23, | ||
| "total": 2.5 | ||
| } |
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.
Not sure why durations_in_seconds was added everywhere. Was the feature added after 2024-05-13?
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.
Yes, the time tracking for test validations was added as of few months ago - thanks to @tiurin ⏲️.
The purpose of it was to use that data to track slow tests or slow services overall.
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.
Context PR here: #12638
| "MessageId": "<uuid:1>", | ||
| "ReceiptHandle": "<receipt-handle:1>", | ||
| "MD5OfBody": "<m-d5-of-body:1>", | ||
| "Body": "\"Command is true!\"" |
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.
Localstack used to give back "Body": "\"Command is True!\""
1198117 to
be0f580
Compare
Test Results - Alternative Providers380 tests 248 ✅ 3m 51s ⏱️ Results for commit b96ac19. ♻️ This comment has been updated with latest results. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 1h 25m 46s ⏱️ Results for commit b96ac19. ♻️ This comment has been updated with latest results. |
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.
LGTM! Nice and neat fix! Thanks a lot for jumping on this, adding the snapshot validated test case and re-validating other tests against AWS 🚀
I only have one minor dict regarding the docstring format, but once addressed this is good to merge!
I'll already approve the PR as this only relates to a docstring, but it would be nice to get it addressed before merging if possible 🙏
| The transformed string with placeholders replaced by values from `replacements`. | ||
| """ | ||
| ... | ||
| is_json_template = template.strip().startswith("{") |
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.
praise: I think this is a nice change to keep the logic encapsulated, as only replace_template_placeholders needed that information. Thanks for making this nicer 👍
| """ | ||
| Replaces placeholders in an EventBridge-style InputTemplate string. | ||
| Args: | ||
| template: The template string containing placeholders like <$.foo.bar>. | ||
| replacements: A dict providing values to fill in. | ||
| Returns: | ||
| The transformed string with placeholders replaced by values from `replacements`. | ||
| """ |
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.
nit: thanks for adding more detailed docstring! we do not use the Google docstring format inside our codebase, and we use Sphinx: https://sphinx-rtd-tutorial.readthedocs.io/en/latest/docstrings.html
This would look like:
"""
Replaces placeholders in an EventBridge-style InputTemplate string.
:param template: The template string containing placeholders like <$.foo.bar>.
:param replacements: A dict providing values to fill in.
:return: The transformed string with placeholders replaced by values from `replacements`.
"""If you could update for coherence before merging, this would be awesome 👍
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 the info! Addressed b96ac19
Motivation
While testing Event Bridge input transformation, realized boolean values were not properly rendered.
Localstack was formatting them with an upper case
TrueandFalsebut AWS formats them astrueandfalse.Changes
Added a special case for boolean values in the placeholder substitution code and added a parity test case.