Add support for ingest pipelines #118
Conversation
|
Hi @dadoonet, could you please give some estimation or feedback to the PR if it makes sense for you and your are going to review it or if I should use my fork project instead? Thx. |
|
@HJK181 Definitely something I'd look to review. |
|
Could you create another PR or edit this one and remove the testcontainer change? BTW if the change of the mapping/templates (removal of Could you do that? Thanks a ton! |
|
I'll refactor and create two separate PRs for testcontainers and pipeline support. The change on the mappings template was necessary as Elasticsearch removed mapping types: removal-of-types.html. I don't understand why the tests did not fail for you as you already used Elastic 7.7.1 with the fabric8 docker image. I got exceptions with testcontainers that |
|
If I remove the testcontainer changes from this branch, the test will fail. If it were not a fork I would create another PR for the 1673bae into the test-containers branch of PR 121. However, this is not possible. So do you prefer me to delete the commit from this branch and ignore the failing tests, or should I wait until PR 121 is merged into master and than refactor this one? |
dadoonet
left a comment
dadoonet
left a comment
There was a problem hiding this comment.
In general this project uses spaces instead of tabs. Could you fix it?
I made a first pass. Could you remove the change for TC and just keep here the pipeline change? Also format using spaces and not tabs so much less changes will be part of your PR.
Thanks a lot!
|
|
||
| By default, Beyonder will not overwrite a pipeline if it already exists. | ||
| This can be overridden by setting `force` to `true` in the expanded factory method | ||
| `ElasticsearchBeyonder.start()`. |
There was a problem hiding this comment.
This line is misplaced as it is still related to index templates. Could you move it back to line 203?
| <commons.io.version>2.7</commons.io.version> | ||
| <jackson.version>2.11.0</jackson.version> | ||
|
|
||
| <!-- For integration tests using Docker or external cluster --> |
src/main/java/fr/pilato/elasticsearch/tools/pipeline/PipelineElasticsearchUpdater.java
Show resolved
Hide resolved
src/main/java/fr/pilato/elasticsearch/tools/pipeline/PipelineElasticsearchUpdater.java
Show resolved
Hide resolved
src/main/java/fr/pilato/elasticsearch/tools/pipeline/PipelineFinder.java
Show resolved
Hide resolved
| protected static Client transportClient; | ||
|
|
||
| @BeforeClass | ||
| public static void initilizeElasticSearch() throws IOException, InterruptedException { |
There was a problem hiding this comment.
Please revert all this as this will be reviewed in #121
| } | ||
|
|
||
| @Test | ||
| public void testPipeline() throws Exception { |
There was a problem hiding this comment.
I think that only this change is needed.
|
Hey @HJK181 Do you still want to get this change in? |
|
HI @dadoonet , sry didn't find the time lately to work on your requested changes. It's still on my list though. |
|
Great to hear. No worries 😉 |
|
Closing in favor of #127 |
2 participants
Adds support for ingest pipelines when using the RestClient.
If you accept the PR, it would be cool if you can make a release out of it as we need it in our project :)