Closed michaelkirk closed 8 months ago
Ok with https://github.com/pelias/config/pull/142 merged, I've rebased and referenced the released config package, so this is ready for review.
I've also just successfully re-run the portland-metro tests with this branch: https://github.com/michaelkirk-pelias/docker/tree/mkirk/drop-es6/projects/portland-metro
Nice, the only things which caught my eye are renaming of edgeNGram
and removal of doc_values
.
I'll just double check the compatible versions tomorrow but otherwise it's pretty straightforward.
In case you didn’t see it, I made some notes in the individual commits.
Code review looks great :+1:
I ran npm run integration
against two versions in the 7.x family and it passed.
When I generated a new pelias/elasticsearch:8.12.1-beta
image I wasn't able to connect to the instance to run the integration tests, the old elasticsearch JS client we use only supports the API up to version 7.x
and also seems to lack support for the modern authentication method.
What was your intention for this PR, merging it now will drop support for 6.x but not add support for 8.x, was that your intent?
Would you consider making the elasticsearch client changes here too so that this lib supports both 7.x
and 8.x
before merging this and proceeding with other repos?
I guess https://github.com/pelias/schema/pull/488/commits/110ac78a394fa3683356d13cbc4b17697215b78b is what I'm missing?
Splitting out was at my request. Both just to make smaller easier to test, encapsulated PRs, and because doing the work of dropping old config options is very different than upgrading an NPM module.
And, honestly, because I still kinda don't love how the new Elasticsearch JS module does versioning, but I'm over that now 😆 and fortunately as 110ac78
shows the changes required are quite small.
I think this is probably good to merge, we'll just run a few other checks. The main thing is we have to make sure this PR generates a new major version of the schema module and is a breaking change. @michaelkirk in case you didn't figure it out (you figure out most things about Pelias 😁), we use semantic versioning (the concept) and semantic-release (the NPM package) to automatically generate changelogs and determine how package versions should from our git commit messages. It's pretty handy when it works and we get the syntax right.
For a major version change we need the "title" of the commit message to include something like feat(something): Change some part of this repo that is backwards incompatible
. And then the "body" of the commit message needs to start with the text "BREAKING CHANGE:". It's a bit tricky, the main thing is to NOT use git commit -m
so that git opens your editor window. The title is line one, line two is always an empty line, and line 3 is the start of the body.
This is mostly informational/for the future, as we can also put this in as maintainers when we merge a PR. It works in the commit body of the merge commit, and we can edit that in Github
What was your intention for this PR, merging it now will drop support for 6.x but not add support for 8.x, was that your intent?
As @orangejulius alluded: Yes, that's the intention. Said differently, this PR is all the es8 changes that are backwards compatible with our legacy elasticsearch client.
I guess https://github.com/pelias/schema/commit/110ac78a394fa3683356d13cbc4b17697215b78b is what I'm missing?
There's a few more changes required for es8 - but they aren't compatible with our legacy elasticsearch client. We'll need everything in #488, which is a superset of this PR. My plan would be to rebase it after this gets merged. If you want me to go about it in a different way let me know. I know it is kind of a big burger to eat all at one time, so if you have ideas about how to better split it up let me know.
semantic-release (the NPM package) to automatically generate changelogs
Thanks @orangejulius - I didn't know about semantic-release. Would you like me to amend the existing commit messages and force push or would you prefer I push another commit with that message format onto the branch?
@michaelkirk can you please allow me to push changes to your branch so I can resolve this conflict and merge this PR 🙏 ?
can you please allow me to push changes to your branch
Sorry I don't have that button.
I think this is because I forked the repositories into an organization (michaelkirk-pelias) rather than my personal account. The docs say (emphasis mine):
People with push access to the upstream repository of a fork owned by a personal account can commit to the forked branches.
At the time I thought having a namespaced organization for the several pelias repositories was a clever way to keep my personal namespace tidy. But as "clever" usually goes...
Anyway, I've merged main and repushed!
Feel free to do whatever is expedient in the future if you want to mangle any of my commits or branches. I don't have a lot of propriety for any of "my" commits making it in so long as the work moves forward.
Here's the reason for this change :rocket:
I'd like Pelias to run on elasticsearch 8 some day. This is a step towards that.
Pelias does not support es6. Some things first deprecated in es6 and compatible with es7 are now breaking with my es8 tests.
This PR addresses those things as well as some other related cleanup I came across in the process.
Here's what actually got changed :clap:
As suggested by @orangejulius, this PR is a subset of #488, including only the changes that are compatible with our existing elasticsearch library, which allows a more focused PR review.
Each commit is intended to address one change. I can break each commit up into separate PR's if you'd prefer.
This is a draft because it depends on the unreleased https://github.com/pelias/config/pull/142update: merged and rebased.Here's how others can test the changes :eyes:
I've run the portland-metro example tests against a docker container built on top of this branch. Running it against more examples might be helpful.
I've manually run
node scripts/list_analyzers.js
andnode scripts/output_mapping.js
and the output looks reasonable, but I've never actually used those scripts for a purpose. If you are familiar with them, it'd be good to double check that they're giving you what you want.