Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(pelias.json): remove explicit esclient.apiVersion from projects #181

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

Conversation

missinglink
Copy link
Member

@missinglink missinglink commented Mar 26, 2020

all the projects are going to start breaking now for the same reason as this Travis run failed.

we've got two options, either:

  • change all the esclient.apiVersion blocks to 7.6 or 7.x
  • remove them all and rely on the default pelias/config value.

I chose the latter because it simplifies the configs.

This relies on pelias/config#126 to be merged first!

@orangejulius
Copy link
Member

If I had to choose, I'd update everything to 7.x.

My rationale for changing to 7.x:

  • Leaving the value in the pelias.json configs, while often redundant, also allows people to make changes to it more easily
  • Elasticsearch 8 will be coming out in the not too distant future (they already have an alpha release), so it will be nice to be able to quickly and easily swap between versions for development, as well as to upgrade the project Elasticsearch versions only as we feel confident, as we've done with previous version upgrades.
  • changes in pelias/config can come through inconsistently into the Docker images unless we take deliberate action to bump all the importer/service Docker images. A nice thing about having the apiVersion in these pelias.json files is that we can smooth over that if we need to.
  • My guess is that they are going to remove the 7.x version when Elasticsearch 8 comes out, so it could also be nice to be able to handle that quickly and easily in the docker projects.

As far as I can tell, there's no option that doesn't us require to occasionally make changes in pelias/config and/or this project to account for the way they manage the apiVersion param in the elasticsearch-js-legacy module, which is frustrating.

But it also means that ultimately we shouldn't spend too much time on thinking about this, since we will be changing things in the future regardless. So I'm happy to see this PR merged as is if you prefer.

@orangejulius
Copy link
Member

It turns out this PR was only required due to an unintentional breaking change in elasticsearch-js-legacy. I'd prefer to keep apiVersion in all the projects so that when we do upgrade Elasticsearch in the future it's clear and explicit. So I think we can close this PR.

@missinglink any thoughts?

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

Successfully merging this pull request may close these issues.

None yet

2 participants