search

brefphp / symfony-messenger

Bridge to use Symfony Messenger on AWS Lambda with Bref
MIT License
72 stars 22 forks source link

Added TransportNameResolver for sns and sqs for automatic transport r… #84

Closed robinlehrmann closed 7 months ago

robinlehrmann commented 9 months ago

Hello :)

This will resolve #83

I thought it might be a good idea to parse the transports mapping in the messenger.yaml and map the transport with the eventSourceARN of a sqsEvent or the EventSubscriptionArn of a snsEvent with the configured transport.

With this solution the $transportName is not required anymore and I don't need to create one lambda per sqs queue for my use case :) Would be great if you can take a look at this @mnapoli

tyx commented 8 months ago

Hi !

I get your point but this is clearly a BC break according to your current implementation. We may continue to support old way and add possibility to introduce your concept that would fallback on old transportName if missing (and it would be nullable by defaut) ?

mnapoli commented 8 months ago

Could you add documentation to help understand this? Hard for me to understand right now TBH.

Also good point, we want to avoid BC breaks 👍

robinlehrmann commented 8 months ago

@tyx yes, this would be a BC break :) I added the $transportName again. I also added some tests as well, to make sure both ways (automatic detection and manually set) works as expected.

In an other merge request I would refactor the SnsConsumerTest if you agree and use Prophecy as well to have only one way of creating mocks. But this is for later. Could you take a look again?

@mnapoli Yes, sure. It's in the README.md file now, let me know if this is understandable :)

And by the way should I implement the transport detector for EventBridge as well because I forgot that one? :) It seams to be not possible to do it with EventBridge, because there is no eventSource or something like this.

robinlehrmann commented 8 months ago

@tyx @mnapoli when are you planning to merge this? :)

tyx commented 8 months ago

Hi ! I'm just a bref user like you ;)

I'm sure Matthieu will do his best to review as soon as he has some free time

robinlehrmann commented 8 months ago

Hello @mnapoli I don't want to pressure, but can you take a look at this, it's quite urgent 😅 would be really nice if this can be merged. Thank you!

SinaFetrat commented 7 months ago

Hi @mnapoli , i was wondering if there is any update or plan about merging this? because I also have the same problem and this will help a lot to get rid of many lambdas

mnapoli commented 7 months ago

Merging since we have now 3 different users asking for it. It's always hard to judge how actually used/useful a feature is when not using it, I've made the mistake of merging stuff for 1% of users before and regretted it 😬

Also I have a rule not to merge stuff I don't understand or can't maintain. I'm breaking that rule here, I trust you guys to have reviewed and maintain it if you face issues 😅

mnapoli commented 7 months ago

I have no idea why but it seems GitHub Actions didn't run on this PR? On master PHPStan fails now, any idea if it's related to this PR?

robinlehrmann commented 7 months ago

I will take a look on this :) @mnapoli

robinlehrmann commented 7 months ago

@mnapoli I ignored the error here: https://github.com/brefphp/symfony-messenger/pull/85/files is this ok for you?