Skip to content
This repository has been archived by the owner on May 28, 2023. It is now read-only.

Rename ES7 docker image service to match existing ES config #523

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

jahvi
Copy link
Contributor

@jahvi jahvi commented Sep 27, 2020

The default docker setup expects the ES container to be named elasticsearch so this should fix the issue where the API cannot connect to the ES server. I also renamed the service node to es1 es that's the name the docker-compose.nodejs.yml depends on.

Fixes #509

Copy link
Contributor

@Fifciu Fifciu left a comment

Choose a reason for hiding this comment

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

I agree with changing container_name but why did you change service's name? I see no point there

@jahvi
Copy link
Contributor Author

jahvi commented Sep 27, 2020

Because the docker-compose.nodejs.yml file depends on a container called es1 or else it won't start.
https://github.com/DivanteLtd/vue-storefront-api/blob/master/docker-compose.nodejs.yml#L9

I only decided to rename it to match the already existing ES v5 container, so it's easy to change between them.

Copy link
Contributor

@Fifciu Fifciu left a comment

Choose a reason for hiding this comment

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

Ah, you are right!
But please rebase it to the develop at first. We will merge develop to master in the new release

@jahvi jahvi changed the base branch from master to develop September 28, 2020 09:42
@jahvi jahvi force-pushed the elasticsearch7-config-fix branch from 24b5436 to df80569 Compare September 28, 2020 09:43
@jahvi jahvi force-pushed the elasticsearch7-config-fix branch from df80569 to 4efb4c2 Compare September 28, 2020 09:47
@jahvi
Copy link
Contributor Author

jahvi commented Sep 28, 2020

@Fifciu No problem, should be fixed now.

@@ -1,12 +1,12 @@
version: '3.0'
services:
es7:
container_name: es7
es1:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this is in conflict for people that my want to run both at the same time as an migration step? So es5 and es7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true, however you can still start/stop the containers when testing
both.

With the current approach you need to change the container names in the
docker files and rebuild them so there's an extra step there.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ES7: docker container name should be 'elasticsearch' instead of 'es7'
3 participants