Skip to content
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

[SFN] Fix Unknown Service sfn errors #10631

Merged
merged 3 commits into from
Apr 10, 2024
Merged

Conversation

MEPalma
Copy link
Contributor

@MEPalma MEPalma commented Apr 10, 2024

Motivation

The current version of the SFN v2 interpreter lacks the capability to enhance aws-sdk failures if the service name of the resource isn't associated with any explicit bindings or isn't available in boto's service catalog. As a result, it generates unknown service error messages, causing the interpreter to fail without reporting the decorated aws-sdk source error.

This pull request aims to resolve this issue by refining the logic for decorating service error names to accurately search for a normalised error name. In cases where none is found, it logs such incidents and returns the original service name as an error. This adjustment is intended to enable the computation to continue, albeit with slightly different cause and error string formatting, while also emphasising the necessity for more explicit normalised error name bindings.

Furthermore, the PR includes bindings for aws-sdk stepfunctions error names along with relevant snapshot tests, addressing issue #10622.

Changes

  • revised logic for decorating aws-sdk service error names
  • added normalised error name binding for aws-sdk stepfunctions
  • added relevant aws-sdk stepfunctions snapshot tests

@MEPalma MEPalma added the semver: patch Non-breaking changes which can be included in patch releases label Apr 10, 2024
@MEPalma MEPalma added this to the 3.4 milestone Apr 10, 2024
@MEPalma MEPalma requested a review from joe4dev April 10, 2024 12:41
@MEPalma MEPalma self-assigned this Apr 10, 2024
Copy link

github-actions bot commented Apr 10, 2024

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 30m 39s ⏱️ - 1m 33s
2 845 tests +5  2 561 ✅ +5  284 💤 ±0  0 ❌ ±0 
2 847 runs  +5  2 561 ✅ +5  286 💤 ±0  0 ❌ ±0 

Results for commit f9c41e7. ± Comparison against base commit ba8d36f.

♻️ This comment has been updated with latest results.

Copy link
Member

@joe4dev joe4dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Thanks for the quick fix @MEPalma 🚀

Your call: Feel free to merge to get the fix out and clean up the test using parametrization afterward.


class StateTaskServiceAwsSdk(StateTaskServiceCallback):
_NORMALISED_SERVICE_NAMES = {"dynamodb": "DynamoDb"}
# Defines bindings of lower-cased service names to the StepFunctions service name included in error messages.
_SERVICE_ERROR_NAMES = {"dynamodb": "DynamoDb", "sfn": "Sfn"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄 lovely AWS consistency

@@ -139,6 +139,63 @@ def test_dynamodb_put_update_get_item(
exec_input,
)

@markers.snapshot.skip_snapshot_verify(
paths=[
# TODO: aws-sdk SFN integration now appears to be inserting decorated error names into the cause messages.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice explaining comments about what are the next steps to fix this 🏅

]
)
@markers.aws.validated
def test_sfn_send_task_success_no_such_token(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

success and failure could be two parameters of a parametrized test with identical code. See parametrization comment in #10168

@MEPalma MEPalma merged commit 8fd2937 into master Apr 10, 2024
30 checks passed
@MEPalma MEPalma deleted the MEP-sfn-unknown_service_sfn branch April 10, 2024 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants