Skip to content

Conversation

@whummer
Copy link
Member

@whummer whummer commented Oct 12, 2022

Add error response for invalid SQS batch message IDs (AWS parity).

This small issue surfaced when running some experiments in "hybrid mode" against real AWS (pointing Lambda event source listeners at an SQS queue in real AWS) - we were basically passing the ReceiptHandle for the message ID on batch deletion in the event source listener.

Also added a small snapshot test to cover this functionality. The PR adds a snapshot transformer to remove the Detail section of returned error responses - could be useful for some of the other skip_snapshot_verify marked tests as well.. 👍 (perhaps could be turned into a fixture, to simplify things a bit..). @thrau @dominikschubert

@whummer whummer temporarily deployed to localstack-ext-tests October 12, 2022 13:40 Inactive
@whummer whummer changed the title Add error message for invalid SQS batch message IDs Add error response for invalid SQS batch message IDs Oct 12, 2022
@whummer whummer changed the title Add error response for invalid SQS batch message IDs [wip] Add error message for invalid SQS batch message IDs Oct 12, 2022
@github-actions
Copy link

github-actions bot commented Oct 12, 2022

LocalStack integration with Pro

       3 files  ±0         3 suites  ±0   1h 16m 14s ⏱️ -10s
1 407 tests +1  1 223 ✔️ ±0  184 💤 +1  0 ±0 
2 005 runs  +1  1 589 ✔️ ±0  416 💤 +1  0 ±0 

Results for commit 9bac489. ± Comparison against base commit c020f64.

♻️ This comment has been updated with latest results.

@whummer whummer temporarily deployed to localstack-ext-tests October 12, 2022 18:00 Inactive
@whummer whummer temporarily deployed to localstack-ext-tests October 12, 2022 20:33 Inactive
@whummer whummer changed the title [wip] Add error message for invalid SQS batch message IDs Add error message for invalid SQS batch message IDs Oct 12, 2022
@coveralls
Copy link

coveralls commented Oct 12, 2022

Coverage Status

Coverage decreased (-0.007%) to 79.166% when pulling 9bac489 on sqs-msg-ids into c020f64 on master.

@whummer whummer temporarily deployed to localstack-ext-tests October 12, 2022 21:33 Inactive
Copy link
Member

@thrau thrau left a comment

Choose a reason for hiding this comment

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

LGTM! just a minor comment regarding the test. no need to change it now, but maybe we can use parameterization for future tests.

@whummer whummer temporarily deployed to localstack-ext-tests October 13, 2022 17:22 Inactive
@whummer whummer temporarily deployed to localstack-ext-tests October 13, 2022 17:23 Inactive
@whummer whummer merged commit bc6f7f7 into master Oct 13, 2022
@whummer whummer deleted the sqs-msg-ids branch October 13, 2022 18:14
cmoralesmx pushed a commit to cmoralesmx/localstack that referenced this pull request Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants