Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork4.5k
[SFN][TestState] Make roleArn optional#13459
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Test passes just fine as roleArn became optional.Also, test runs way faster against AWS now because the majority of time was spent in role creation.
Hack to check if basic test state scenario works well in LocalStack without a real role.Pending comment about a hack and a validation for states where dummy role cannot be used.
When state is Task and mock is not supplied
Faster execution with only necessary fields provided. Snapshot recording hasn't changed.
| sfn_snapshot.match("test_case_response",test_case_response) | ||
| @markers.aws.validated | ||
| deftest_base_lambda_service_task_state_no_role_arn_validation( |
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.
Test for the case whenroleArn is actually needed.
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 22m 32s ⏱️ - 1h 40m 51s Results for commit9760b97. ± Comparison against base commit049201f. This pull requestremoves 3401 andadds 1 tests.Note that renamed tests count towards both. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 39m 10s ⏱️ Results for commit9760b97. |
gregfurman left a comment
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.
Looks fine to unblock us 👍
| arn=stepfunctions_state_machine_arn( | ||
| name=name,account_id=context.account_id,region_name=context.region | ||
| ) | ||
| role_arn=request.get("roleArn") |
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.
Do you know if theroleArn param requires any special validations?
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 plan to investigate it later, also noticed the lack of validation.
84bdd4a intomainUh oh!
There was an error while loading.Please reload this page.
Motivation
roleArnis now only required when tested state is Task and a mock is not provided.Closes DRG-238.
Changes
Add validation for
roleArnpresence.Add workaround for
roleArnbeing not optional in Execution hierarchy.Remove roleArn from tests where it is not needed - much faster execution agaisnt AWS with only necessary fields provided. Snapshot recording hasn't changed.
Tests
Related