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)?
.github/workflows/ci.yml
Outdated
| run: | | ||
| pip install localstack localstack | ||
| docker pull localstack/localstack-pro | ||
| localstack auth set-token $LOCALSTACK_AUTH_TOKEN |
There was a problem hiding this comment.
question: I am a bit confused by this change. Why would you stop setting an auth token when switching to Pro explicitly?
There was a problem hiding this comment.
Because the LocalStack CLI detects if the LOCALSTACK_AUTH_TOKEN env var is set. For the case that it is, it will start the Pro image without extra arguments. The idea of the removal was to save 1 line of code. But I can add it it back if you prefer it.
.github/workflows/ci.yml
Outdated
| @@ -59,7 +59,6 @@ jobs: | |||
| run: | | |||
| pip install localstack localstack | |||
There was a problem hiding this comment.
nit: This is out of scope fore this PR, bit it definitely is weird 😄 Why is the package listed here twice?
There was a problem hiding this comment.
CI triaging shenanigans 😅 . The awslocal-cli (the wrapper around aws-cli) is not necessary for the testing.
src/index.js
Outdated
| env.DOCKER_CMD = 'sudo docker'; | ||
| } | ||
| const options = { env: env, maxBuffer }; | ||
| return exec('localstack start -d --image localstack/localstack-pro', options) |
There was a problem hiding this comment.
question: Similar to the comment above, this change seems to be actually moving back from using Pro to Community (at least for as long as localstack/locastack is still referencing Community).
But I guess it would be fine, IF a user has set an auth token and its forwarded here correctly. Is this the case?
There was a problem hiding this comment.
There is no --image parameter. This was Claude hallucinating.
|
@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 👍 |
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