Skip to content

feat: Allow SQS port number to be passed as environmental configuration#660

Merged
ahmedelbougha merged 5 commits into
masterfrom
cofigurable-offline-sqs-port-number
Feb 18, 2022
Merged

feat: Allow SQS port number to be passed as environmental configuration#660
ahmedelbougha merged 5 commits into
masterfrom
cofigurable-offline-sqs-port-number

Conversation

@ahmedelbougha
Copy link
Copy Markdown
Contributor

Please refer to #659 SQS Port as Env. Configuration for more details.

Copy link
Copy Markdown
Contributor

@seb-cr seb-cr left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks for doing this!

I wondered about allowing the port number to be included in LAMBDA_WRAPPER_OFFLINE_SQS_HOST, e.g. localhost:4566, so that we avoid another SQS config variable. However I think this would be harder to implement in a backwards compatible way, so I'm happy with your approach.

Three more small things needed here:

  • Please add a test for this in tests/unit/Service/SQS.service.test.js. There's a suite of queue URL tests in there.
  • Also add a note to the readme documenting use of LAMBDA_WRAPPER_OFFLINE_SQS_PORT. This can go in the Local SQS mode section.
  • Finally, you'll need to prefix your PR title with feat: so that semantic-release publishes this when merged.

@ahmedelbougha ahmedelbougha changed the title Allow SQS port number to be passed as environmental configuration feat: Allow SQS port number to be passed as environmental configuration Feb 18, 2022
@ahmedelbougha ahmedelbougha requested a review from seb-cr February 18, 2022 14:37
@ahmedelbougha
Copy link
Copy Markdown
Contributor Author

Thanks @seb-cr for comprehensive review and comments.
I have implemented the comments you mentioned.

@seb-cr
Copy link
Copy Markdown
Contributor

seb-cr commented Feb 18, 2022

Thanks @seb-cr for comprehensive review and comments. I have implemented the comments you mentioned.

Great, thank you!

Just checking your tests, they need to be slightly more robust – I can make them fail if I set LAMBDA_WRAPPER_OFFLINE_SQS_PORT in my terminal. Try running this:

$ LAMBDA_WRAPPER_OFFLINE_SQS_PORT=1234 yarn test tests/unit/Service/SQS.service.test.js

Make sure at the start of each test you delete any environment variables that you expect to be unset.

There are also beforeAll and afterAll hooks at the top of the suite that save and restore the process.env values that get set/deleted by the tests. LAMBDA_WRAPPER_OFFLINE_SQS_PORT and LAMBDA_WRAPPER_OFFLINE_SQS_HOST should be added there.

…ER_OFFLINE_SQS_PORT=1234 and LAMBDA_WRAPPER_OFFLINE_SQS_HOST=custom1-host
@ahmedelbougha
Copy link
Copy Markdown
Contributor Author

Great @seb-cr, I have implemented that as well.

Copy link
Copy Markdown
Contributor

@seb-cr seb-cr left a comment

Choose a reason for hiding this comment

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

All looks good. Nice work!

@ahmedelbougha ahmedelbougha merged commit 6ff4f78 into master Feb 18, 2022
@ahmedelbougha ahmedelbougha deleted the cofigurable-offline-sqs-port-number branch February 18, 2022 16:49
@ahmedelbougha
Copy link
Copy Markdown
Contributor Author

All looks good. Nice work!

Thank you.

@github-actions
Copy link
Copy Markdown

🎉 This PR is included in version 1.10.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants