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

feat: add globalOptions support in SqsService with sqs endpoint property #81

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

marcin-zajac-pc
Copy link

Context

Related issue: Issue #78.

Problem

Currently, we're unable to set the SQS endpoint when the module is registered. As mentioned in the issue, when using an emulator, we receive warnings because the queueUrl is composed from AWS credentials in the environment, which differs from local URLs such as those used by LocalStack.

QueueUrl=https://localhost.localstack.cloud:4566/000000000000/iam_sync-user-queue differs from SQSClient resolved endpoint=https://sqs.us-east-1.amazonaws.com/, using QueueUrl host as endpoint.
Set [endpoint=string] or [useQueueUrlAsEndpoint=false] on the SQSClient

The error message suggests using the useQueueUrlAsEndpoint property, but I couldn't find that in the SQSClient options. However, setting the endpoint directly resolves the issue.

To provide more background, when running SQS locally using LocalStack, I set the SQS endpoint to http://localhost:4566.

Solution

I added an optional globalOptions object property with an endpoint field to allow setting the endpoint directly on the SQSClient. The condition ensures that if the consumer/producer options have an sqs property, it won't be overwritten by the global one. I decided that consumer/producer options should take priority.

This solution was tested locally and works as expected.

Other Thoughts

We can discuss adding more properties to the globalOptions. I'm open to suggestions, but for now, I wasn't sure if anything else would be necessary.

@ssut
Copy link
Owner

ssut commented Jul 12, 2024

Thanks for addressing the issue with this PR.
All the changes seem LGTM. However I think it would be helpful to write some docs for this change so that new users of this library will know how to use this option. Could you please add some docs for this?

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.

5 participants