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

[Messenger] added support for Amazon SQS QueueUrl as DSN #37306

Merged
merged 1 commit into from
Jun 22, 2020
Merged

[Messenger] added support for Amazon SQS QueueUrl as DSN #37306

merged 1 commit into from
Jun 22, 2020

Conversation

starred-gijs
Copy link
Contributor

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #37305
License MIT
Doc PR N/A

This will allow to use an Amazon SQS QueueUrl as transport DSN

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Should Connection::fromDsn() set Connection::$queueUrl directly when it is given a queue url (to avoid calling getQueueUrl() on the SQS client when we already have it)?

@chalasr chalasr added this to the next milestone Jun 16, 2020
@starred-gijs
Copy link
Contributor Author

starred-gijs commented Jun 17, 2020

Should Connection::fromDsn() set Connection::$queueUrl directly when it is given a queue url (to avoid calling getQueueUrl() on the SQS client when we already have it)?

Did not consider that to be part of the purpose of this PR. I rather keep this PR as minimal as possible.
The following DSN was already supported,
sqs://sqs.us-east-2.amazonaws.com/123456789012/ab1-MyQueue-A2BCDEF3GHI4
this PR only extends that to support https DSN
https://sqs.us-east-2.amazonaws.com/123456789012/ab1-MyQueue-A2BCDEF3GHI4

@RocKordier
Copy link
Contributor

@starred-gijs for consistency reasons maybe also this check

} elseif (0 === strpos($dsn, 'sqs://')) {

should be adjusted.
What do you think?

@chalasr
Copy link
Member

chalasr commented Jun 18, 2020

Can you please add an entry in https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Messenger/Bridge/AmazonSqs/CHANGELOG.md?

@starred-gijs
Copy link
Contributor Author

Wasn't sure how to update the changelog. Let me know if it needs to be different.

Copy link
Member

@Nyholm Nyholm 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 @starred-gijs

This is a great first contribution.

@chalasr
Copy link
Member

chalasr commented Jun 22, 2020

Thank you @starred-gijs. Congratulations for your first PR on Symfony!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Messenger] Support SQS Transport with QueueUrl as DSN
7 participants