Conversation
alexrashed
left a comment
There was a problem hiding this comment.
Thanks for preparing serverless-localstack for our upcoming image consolidation! But I think there might be a bit of a misunderstanding. I think we should move towards using the Pro image (with an auth token) everywhere now, such that - at the time of the image consolidation - this will just continue to work.
This PR - to me - seems like it's moving to explicitly use community (at least for as long as localstack/localstack is still the OSS version). This would mean that some users who are already using Pro would suddenly start using Community (?!) and for all users this will suddenly break with the image consolidation (because no auth token is set).
In addition, there are also other usages of Community here in the repo which are not touched by this PR:
serverless-localstack/src/index.js
Lines 525 to 539 in e7e53d8
serverless-localstack/.github/workflows/ci.yml
Lines 56 to 61 in e7e53d8
- https://github.com/localstack/serverless-examples/blob/c4874f3cb38d67d64f90cc2d444941c55d8064b9/Makefile#L7-L8
Happy to discuss this directly, but I guess the most important question is: How should this tool look like / act when there is no separation between Community and Pro anymore, and how can this be executed on as soon as possible (rather than breaking at the time of the consolidation or in the worst case even twice)?
|
@alexrashed Thank you so much for the thorough review. As you will notice from my answers to your questions, I was relying heavily on the auto detection of the auth token by the localstack-cli. That was because the docs and the help message of the tool do not describe any parameter to allow the specification of the docker image. Looking at the PR I can see why you assumed I was setting the community version as the main image to use, and made me think that users of the plugin could also be confused. So I searched (and found) in the localstack-cli codebase for a config var that allows the specification of the LocalStack image: So with that in mind. I added the option for users to set the Happy to discuss any more improvements to this pr 👍 |
alexrashed
left a comment
There was a problem hiding this comment.
Thanks a lot for jumping on the comments and adding the new image param and the respective default. I added a comment that I don't think we need the auth token to be required, but once this is changed I think this PR is good to go! 💯
Co-authored-by: Alex Rashed <2796604+alexrashed@users.noreply.github.com>
Motivation
The localstack/localstack image is being migrated to LocalStack Pro. After March 23rd, the community image will no longer work as expected. This update ensures the serverless-localstack plugin
continues to function by switching to localstack/localstack-pro and requiring users to set a LOCALSTACK_AUTH_TOKEN.
Changes
Notes