Skip to content

[ESM] Validate event sources existence #12297

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 3, 2025
Merged

[ESM] Validate event sources existence #12297

merged 1 commit into from
Mar 3, 2025
+196 −9

Conversation

eLRuLL
Copy link
Contributor

@eLRuLL eLRuLL commented Feb 21, 2025 •
edited
Loading

Motivation

This PR allows for the ESM to validate the existence of different event source resources

Changes

  • Adds check for sqs existence in the event source validation
  • Adds check for kinesis streams existence in the event source validation
  • Adds check for dynamodb streams existence in the event source validation

Testing

  • Created the test_esm_with_not_existing_sqs_queue test inside the test_lambda_integration_sqs.py tests

localstack-bot
localstack-bot reviewed Feb 21, 2025
View reviewed changes
Copy link
Contributor

@localstack-bot localstack-bot left a comment

Choose a reason for hiding this comment

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

Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.

@localstack-bot
Copy link
Contributor

localstack-bot commented Feb 21, 2025 •
edited
Loading

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@eLRuLL
Copy link
Contributor Author

eLRuLL commented Feb 21, 2025

I have read the CLA Document and I hereby sign the CLA

@eLRuLL
Copy link
Contributor Author

eLRuLL commented Feb 21, 2025

recheck

localstack-bot added a commit that referenced this pull request Feb 21, 2025
@localstack-bot
@eLRuLL has signed the CLA in #12297
3ea6a22
@k-a-il k-a-il added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Feb 21, 2025
@k-a-il k-a-il requested review from bentsku and k-a-il and removed request for joe4dev, dfangl, gregfurman and dominikschubert February 21, 2025 10:55
@eLRuLL eLRuLL force-pushed the raul/fix-esm-sqs-validation branch from a2099ce to a276f47 Compare February 22, 2025 02:51
@eLRuLL eLRuLL changed the title [ESM] Validate SQS source [ESM] Validate event sources existence Feb 22, 2025
@eLRuLL eLRuLL force-pushed the raul/fix-esm-sqs-validation branch from a276f47 to c8501c4 Compare February 22, 2025 03:58
bentsku
bentsku requested changes Feb 25, 2025
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

Hello @eLRuLL and thanks a lot for your contribution!

Nice extra validation of Event Source Mappings! 🚀 this will help users by failing early in the process, and provide more clarity.

It seems it broke the behavior of Event Source Mappings: there are 43 broken integration tests, and another one in bootstrap that also seems related to the change: it looks like it is not possible to create new valid Event Source Mapping of certain type anymore.

I also have comments about the access to other services, we usually tend to manage inter-service communication via the connect_to utility, so that we have clear boundaries between services.

The tests also do not use the snapshot fixture and it would be great to add it, as it provides a little bit extra parity at no cost.

Thanks again for the contribution!

@eLRuLL eLRuLL force-pushed the raul/fix-esm-sqs-validation branch 2 times, most recently from d159d26 to 263e556 Compare February 26, 2025 03:16
@eLRuLL eLRuLL requested a review from bentsku February 26, 2025 03:16
@eLRuLL eLRuLL force-pushed the raul/fix-esm-sqs-validation branch 2 times, most recently from 7e89ffb to 895fb97 Compare February 26, 2025 14:00
bentsku
bentsku requested changes Feb 26, 2025
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

Thanks for jumping on the comment to fix them! This is really nice, and no tests are broken now! Nice 🚀

Very nice use of the snapshots too!

I just have a few comments: one regarding the creation of the internal client, that misses a bit of internal metadata in order to properly identify the request internally, so we can properly verify permission when LOCALSTACK_ENFORCE_IAM is enabled.

Another comment is regarding the SQS operation used to verify the queue existence. It seems AWS is using a different operation (this needed a bit of investigation which is not too clear on AWS side sadly!)

We could also re-use the exception raised by SQS to return the error message in order to rely on SQS sending us the right error message, as it seems passed as-is by AWS too.

I believe we will be good to go once this is addressed! Thanks again, this is looking great!

@eLRuLL eLRuLL force-pushed the raul/fix-esm-sqs-validation branch from 895fb97 to ce759dd Compare February 28, 2025 02:11
@eLRuLL eLRuLL requested a review from bentsku February 28, 2025 22:15
bentsku
bentsku approved these changes Mar 3, 2025
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! Thanks a lot for addressing the comments, this looks great and very streamlined.

I'd like to get @joe4dev approval on this as he is one of the owners of Lambda and I wouldn't want to miss something, but to me this looks good to go! 🚀

@bentsku bentsku requested a review from joe4dev March 3, 2025 12:16
joe4dev
joe4dev approved these changes Mar 3, 2025
View reviewed changes
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.

Thank you for improving the DevX around Lambda Event Source mappings and dilligently addressing @bentsku comments 👏👏👏
It's so much clearer to catch issues early rather than allowing for creating broken ESMs 🚀

k-a-il
k-a-il approved these changes Mar 3, 2025
View reviewed changes
Copy link
Contributor

@k-a-il k-a-il left a comment

Choose a reason for hiding this comment

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

Appreciate your contribution to the Lambda ESM validation!

@bentsku bentsku merged commit dd0d96e into localstack:master Mar 3, 2025
29 checks passed
@eLRuLL eLRuLL deleted the raul/fix-esm-sqs-validation branch March 3, 2025 16:39
@bentsku bentsku mentioned this pull request Mar 4, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bentsku bentsku bentsku approved these changes

@localstack-bot localstack-bot localstack-bot left review comments

@joe4dev joe4dev joe4dev approved these changes

@k-a-il k-a-il k-a-il approved these changes

Assignees
No one assigned
Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@eLRuLL @localstack-bot @joe4dev @bentsku @k-a-il