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

Drop es6 support in pursuit of supporting es8 #490

Merged
merged 8 commits into from
Mar 12, 2024

Conversation

michaelkirk
Copy link
Contributor

@michaelkirk michaelkirk commented Jul 10, 2023

Here's the reason for this change 🚀

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 👏

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 pelias/config#142
update: merged and rebased.


Here's how others can test the changes 👀

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 and node 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.

It was introduced as part of "initial commit" without any particular
explanation:
ee26c2f

I'm **guessing** this existed to facilitate updating es6 to es7.
In es6 _type was recommended to be "_doc", and in es7, _doc was the
only valid value.

For details, see: https://www.elastic.co/guide/en/elasticsearch/reference/7.17/removal-of-types.html
Since we don't support es6, there's no need to specify it in any case,
and removing it paves the way to es8.

See:
https://www.elastic.co/guide/en/elasticsearch/reference/7.17/removal-of-types.html
The rename to edge_ngram originally occurred in 6.4.

Since the minimum supported elasticsearch is 7.4.2, we can safely use
the modern spelling unconditionally.
From: https://www.elastic.co/guide/en/elasticsearch/reference/current/doc-values.html

> Doc values are supported on almost all field types, with the notable
exception of text and annotated_text fields.

The error produced on elastic8.8 was:

> [mapper_parsing_exception] unknown parameter [doc_values] on mapper
[ngram] of type [text]

AFAIK this was never supported, but maybe setting it to "false" was just
silently ignored in older versions of elasticsearch?
I think they've been broken since
b6e92d4, which changed schema to have
a single mapping. Presumably this was done as part of deprecating `doc._type`
@michaelkirk michaelkirk force-pushed the mkirk/drop-es6 branch 6 times, most recently from 53b1e54 to 01d908c Compare February 9, 2024 21:58
@michaelkirk michaelkirk marked this pull request as ready for review February 12, 2024 18:06
@michaelkirk
Copy link
Contributor Author

Ok with pelias/config#142 merged, I've rebased and referenced the released config package, so this is ready for review.

@michaelkirk
Copy link
Contributor Author

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

@missinglink
Copy link
Member

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.

@michaelkirk
Copy link
Contributor Author

michaelkirk commented Feb 12, 2024 via email

@missinglink
Copy link
Member

Code review looks great 👍

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?

@missinglink
Copy link
Member

I guess 110ac78 is what I'm missing?

@orangejulius
Copy link
Member

orangejulius commented Feb 13, 2024

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

@michaelkirk
Copy link
Contributor Author

michaelkirk commented Feb 13, 2024

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 110ac78 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.

@michaelkirk
Copy link
Contributor Author

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?

@missinglink
Copy link
Member

missinglink commented Mar 11, 2024

@michaelkirk can you please allow me to push changes to your branch so I can resolve this conflict and merge this PR 🙏 ?

foo

@michaelkirk
Copy link
Contributor Author

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.

@michaelkirk michaelkirk changed the title Drop support es6 in pursuit of supporting es8 Drop es6 support in pursuit of supporting es8 Mar 11, 2024
@missinglink missinglink merged commit 9fb43bc into pelias:master Mar 12, 2024
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.

3 participants