Depends on D3695
Adding localstack configuration to the docker-compose.yml file.
Details
Details
cd services docker-compose down && docker-compose up localstack
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| services/docker-compose.yml | ||
|---|---|---|
| 65 ↗ | (On Diff #11286) | DEFAULT_REGION is deprecated https://docs.localstack.cloud/localstack/configuration/ |
| 66 ↗ | (On Diff #11286) | This should probably point to a persistent volume. See https://docs.localstack.cloud/localstack/persistence-mechanism/ |
| 67 ↗ | (On Diff #11286) | Is this really necessary? This is the default location and the service seems to run without it. |
Comment Actions
Thanks for linking the docs, @jimpo! Really appreciate the thorough review.
@karol-bisztyga – would be great if you could either:
- Option 1: read the docs thoroughly and make sure you understand what's going on
- Option 2: research every single line you write and make sure you understand it 100%
I mention this because in the past, when you are the first person on the team to use a new technology you haven't always taken the time to learn best practices, and sometimes just submit something that works. This can lead to a situation that is very costly to fix and as a result, the fix gets deferred. This can be addressed ahead of time if you take the time to make sure you are understanding best practices.
| services/docker-compose.yml | ||
|---|---|---|
| 65 ↗ | (On Diff #11286) | right, thanks |
| 66 ↗ | (On Diff #11286) | And why is that exactly? That's all in the docs about DATA_DIR:
Ok, in the snippet below they use the volume environment:
- LOCALSTACK_API_KEY=...
- DATA_DIR=${TMPDIR}/localstack/data
- HOST_TMP_FOLDER=${TMPDIR}
volumes:
- ${TMPDIR}:/tmp/localstackbut earlier they use the line DATA_DIR=/tmp/localstack/data. Is there anything there that says we should use a volume in such a case? Maybe I missed something. The line without using a volume appears in tutorials (1 2) and discussions |
| 67 ↗ | (On Diff #11286) | It can be removed. Thanks. |
Comment Actions
It seems like we'd want a distinct deployment configuration (distinct docker-compose.yml?) for dev vs. prod... curious on what best practices are in that case, particularly in the context of localstack (which is meant to be dev-only, I think)
Three questions for @jimpo:
- Any perspective on the deployment configuration question above?
- Also curious for your perspective on my inline comments below
- Finally, give @karol-bisztyga's most recent comment a read on the persistent volume question
| services/docker-compose.yml | ||
|---|---|---|
| 74–75 ↗ | (On Diff #11602) | Is it better to use expose here? |
| 81 ↗ | (On Diff #11602) | Do we need to declare a depends_on or links relationship for the above services on localstack? |
Comment Actions
Yeah, if you're deploying to prod with docker-compose, I'd just make a separate docker-compose.prod.yml and you can do docker-compose -f docker-compose.prod.yml.
That said, docker-compose isn't really a production deployment tool, unlike swarm or ECS or something, since it only runs on one machine. So longer term it would be good to switch to something like that.
| services/docker-compose.yml | ||
|---|---|---|
| 74–75 ↗ | (On Diff #11602) | I expect it's already exposed in the Dockerfile. |
| 81 ↗ | (On Diff #11602) | links has been deprecated in favor of by networks. depends_on is a good idea though. |
| 66 ↗ | (On Diff #11286) | DATA_DIR just tells it where in the container to store data. Having a volume will ensure that the data is preserved across service restarts. |
| services/docker-compose.yml | ||
|---|---|---|
| 81 ↗ | (On Diff #11602) | The question is: do we want to use only the local cloud for locally launched services? If we want to test the connection to the AWS from the local environment, then we probably don't want this depends_on (as we would want to run the services without having to run the local stack in such a case). |
| 66 ↗ | (On Diff #11286) | I'm not sure if we want to persist the data between the restarts. |
| services/docker-compose.yml | ||
|---|---|---|
| 75 ↗ | (On Diff #11981) | Why bind mount a temp directory instead of using a data volume? It's just gonna create issues with permissions. Also ${TMPDIR} is not used to refer to a path in both the host and the container which is a bad idea. |
Comment Actions
Temp dir makes sense to me
| services/docker-compose.yml | ||
|---|---|---|
| 74–75 ↗ | (On Diff #11602) | I think expose is the same as ports, but it just doesn't expose the port on the host machine. Source on StackOverflow That said, if this is primarily meant for dev configuration then there's no reason to use expose |
| 81 ↗ | (On Diff #11602) | I don't think we want to test the connection to AWS from the local environment. Open to arguments against this, but it seems like we want the dev environment to work fully with localstack, right? |
| services/docker-compose.yml | ||
|---|---|---|
| 81 ↗ | (On Diff #11602) | I think we should discuss this separately, I created a task for this https://linear.app/comm/issue/ENG-1056/discuss-dev-mode-design |
Comment Actions
We have a task for this https://linear.app/comm/issue/ENG-1056/discuss-dev-mode-design
Comment Actions
- That task makes zero mention of depends_on
- It's disappointing that you continue to defer simple feedback from your reviewer rather than just addressing it. We talked about this just yesterday... please, please, please try to get better at this. It is so easy for you to just add depends_on like you were requested to... it boggles my mind that after all the feedback, you still choose to do what is easier for you rather than prioritizing the feedback from your reviewer
Comment Actions
There is an explicit link in this task https://phabricator.ashoat.com/D3692#106752 that leads to your comment and this comment mentions depends_on specifically. If that's too implicit, I can copy-paste this stuff to the task.
- It's disappointing that you continue to defer simple feedback from your reviewer rather than just addressing it. We talked about this just yesterday... please, please, please try to get better at this. It is so easy for you to just add depends_on like you were requested to... it boggles my mind that after all the feedback, you still choose to do what is easier for you rather than prioritizing the feedback from your reviewer
I started a discussion on the task, the decision for adding depends_on depends on its result. Ok, so if I add depends_on here and then in the task we decide it's redundant, then we'd have to remove it again. Doesn't make sense to me, but ok, I'm addressing this feedback immediately then, so we can skip pointless cycles of exchanging opinions, etc: D3958