Support es 7 #266
Support es 7 #266
Conversation
AlexeyRaga
force-pushed
the
support-es-7
branch
from
176ca14
to
20b060f
Compare
AlexeyRaga
force-pushed
the
support-es-7
branch
from
1e7a33d
to
88a0575
Compare
|
@bitemyapp Sorry, I did not realise that Travis is using a different way of provisioning the ES and that there was an example project. I have fixed them both and now the build is green, please have another look. |
| @@ -1,3 +0,0 @@ | |||
| FROM docker.elastic.co/elasticsearch/elasticsearch:5.6.0 | |||
There was a problem hiding this comment.
What's the purpose for deleting stuff behind elasticsearch/?
There was a problem hiding this comment.
Not needed anymore.
It was used in docker-compose only, but now it is possible to just use an official docker image.
|
@AlexeyRaga I'm interested in supporting this and updating Bloodhound to ES 7. I've asked @JoseD92 to combine his ES 7 update effort with what you've done. So even if we don't merge this specific PR in the end, it's likely the final PR will reflect the good work you've done here. |
|
@bitemyapp Where I can see these changes? We are currently maintaining our own fork of |
|
With @JoseD92 PR merged, what holds us back from having ES7 support in |
|
@bitemyapp @AlexeyRaga any update on this ? |
|
that would be awesome, as well as an update on katip-elasticsearch. I can help but would like some confirmation this works first. |
|
cc @wraithm |
src/Database/Bloodhound/Types.hs
Outdated
| parseJSON _ = empty | ||
|
|
||
| data SearchTemplate = | ||
| SearchTemplate { |
There was a problem hiding this comment.
style nitpick: This is a bit different from the other data type definitions in this project. Would you mind changing it?
src/Database/Bloodhound/Types.hs
Outdated
| s .:? "lang" <*> | ||
| s .:? "source" <*> | ||
| s .:? "options" <*> | ||
| v .: "_id" <*> |
There was a problem hiding this comment.
Would you mind cleaning this up a bit?
src/Database/Bloodhound/Types.hs
Outdated
| v .: "found" | ||
| ) | ||
| script | ||
| parseJSON _ = empty |
There was a problem hiding this comment.
Style nit: Please add a newline here
|
I've tried to use this with katip-elasticsearch and this update breaks it. I wonder where it's best to break the interface bloohound vs katip (I don't know enough about either to have an opinion) |
Address style nits from bitemyapp#266
|
@wraithm I've addressed the style nits you raised. It seems that the original author is no longer in a position to maintain this particular PR. If there are any other changes that need to be made, I am happy to have a go - but I will need to re-open this under a fork I control. (For example, it seems that the disabled test could be re-enabled). |
|
Great! Thanks. I'll get to this later today. |
| @@ -98,23 +97,23 @@ upload: | |||
|
|
|||
| ## Run test environment | |||
| compose-ES5: | |||
There was a problem hiding this comment.
Minor naming thing, would you mind changing the name of this (and the others) to compose-ES7? Maybe it should just simply be called compose?
There was a problem hiding this comment.
My personal opinion is that compose makes sense because there is nothing obviously connected to the version here.
There was a problem hiding this comment.
This has been addressed on #276, which is a duplicate PR that I control.
|
@MichaelXavier Ping, do you have any opinions about katip-elasticsearch and this PR? I've reviewed this, and I'm pretty sure this is good to go. |
Changes
docker composeto run ES 7.4.1MappingTypeBloodhound)Note that one tests is currently disabled due to a bug that I have found in the ES itself: elastic/elasticsearch#48670
The bug hits rather rare use case, and
bloodhoundis not at fault so I think it shouldn't be a show stopper for moving forward.