Conversation
pquentin
left a comment
There was a problem hiding this comment.
Thanks! While this changes the directory name, it does not close #62 as it does not change the container names:
Lines 90 to 93 in d48f652
I'm getting the following error when trying it with an instance of start-local already running:
The docker container 'es-local-dev' is already running!
You can have only one running at time.
To stop the container run the following command:
|
@pquentin this is the expected behaviour since we want to have only one Elasticsearch instance running at time. Changing the folder name allows having multiple installations, since we can have different volume name (docker uses the folder name as prefix for the volume). |
pquentin
left a comment
There was a problem hiding this comment.
While you can have different installations, you still need to manually docker rm all containers, otherwise you get:
Error response from daemon: Conflict. The container name "/es-local-dev" is already in use by container "e4d9fb3587bcb97d50daec85144ae2c22189127323a3c6c5a33644156582b1e1". You have to remove (or rename) that container to be able to reuse that name.
Using different names would avoid this problem (but not the port collision). Since you're saying this is by design, I'm approving as the functionality works.
|
@ezimuel |
pquentin
left a comment
There was a problem hiding this comment.
This works well, thank you! I've added two nitpicks that you can ignore.
start-local.sh
Outdated
| elasticsearch_container_name="es-local-dev-${ES_LOCAL_DIR:-}" | ||
| # Kibana container name | ||
| kibana_container_name="kibana-local-dev" | ||
| kibana_container_name="kibana-local-dev-${ES_LOCAL_DIR:-}" |
There was a problem hiding this comment.
Should this be named ES_LOCAL_DIR? It is consistent with ES_LOCAL_VERSION which also applies to Kibana, so that's good.
Still, when I see ES I think about Elasticsearch, so something like ELASTIC_STACK_LOCAL_DIR would be more descriptive but also more verbose.
There was a problem hiding this comment.
The reason for choosing ES_LOCAL_DIR was to be consistent with the existing settings. The ES_LOCAL_VERSION is used also in Kibana since the version must be the same. If you want ES can be think as Elastic Stack, rather than only Elasticsearch 😄
start-local.sh
Outdated
| min_docker_compose="1.29.0" | ||
| # Elasticsearch container name | ||
| elasticsearch_container_name="es-local-dev" | ||
| elasticsearch_container_name="es-local-dev-${ES_LOCAL_DIR:-}" |
There was a problem hiding this comment.
Is there a way to avoid the dash when no local directory is set? This is what most users will see:
✔ Container es-local-dev- Healthy
✔ Container kibana-settings- Exited
✔ Container kibana-local-dev- Healthy
There was a problem hiding this comment.
Good catch! I removed the dash at the end with this syntax:
elasticsearch_container_name="es-local-dev${ES_LOCAL_DIR:+-${ES_LOCAL_DIR}}"
There was a problem hiding this comment.
Thanks! The only difference now is that docker rm fails on uninstall.sh but that's OK as the script still uninstalls correctly.
Do you want to remove the following Docker images?
- docker.elastic.co/elasticsearch/elasticsearch:9.0.3-arm64
- docker.elastic.co/kibana/kibana:9.0.3-arm64
Do you confirm? (yes/no)
yes
Failed to remove image docker.elastic.co/elasticsearch/elasticsearch:9.0.3-arm64. It might be in use.
Failed to remove image docker.elastic.co/kibana/kibana:9.0.3-arm64. It might be in use.
Start-local successfully removed
pquentin
left a comment
There was a problem hiding this comment.
Thank you for iterating! LGTM.
This PR adds the
ES_LOCAL_DIRenvironment variable to change the installation folder name (default iselastic-start-local).You can specify this ENV variable as follows:
This PR implements the request in #62.