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

Add docker files for easier development setup #7

Merged
merged 2 commits into from
Jan 17, 2023

Conversation

ssaarinen
Copy link

Helps to whip up elasticsearch with freshly built raudikko plugin.

Maybe even better would be to run some integration tests in the build w/ e.g. https://www.testcontainers.org/modules/docker_compose/ but this helped me do manual testing with ES8 support.

etc/Dockerfile Outdated
FROM elasticsearch:${ES_VERSION}
# Needs to repeat the ARG here or it won't work
# https://docs.docker.com/engine/reference/builder/#understand-how-arg-and-from-interact
ARG ES_VERSION=8.5.3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be sufficient to just say ARG ES_VERSION here without repeating the version?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like it from the docs I linked but apparently did not read :D Will fix that

GET http://localhost:9200

### show plugins
GET http://localhost:9200/_cat/plugins?v=true&s=component&h=component,version,description
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be useful to have actually exercise the code path of the plugin here. Perhaps making the same call as in the example of README.md:

GET http://localhost:9200/_analyze
Content-Type: application/json

{
    "tokenizer": "finnish",
    "filter": [
        {
            "type": "raudikko"
        }
    ],
    "text": "Testataan raudikon analyysiä tällä tavalla yksinkertaisesti."
}

Copy link
Author

@ssaarinen ssaarinen Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that the exact copy of this already?

curl -XGET 'localhost:9200/_analyze' -H 'Content-Type: application/json' -d '
{
"tokenizer" : "finnish",
"filter" : [{"type": "raudikko"}],
"text" : "Testataan raudikon analyysiä tällä tavalla yksinkertaisesti."
}'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is. I actually generated the request by pasting the original curl request into IDEA.

My point is that if we're going to have a file of executable http-requests, why not include this there? It would feel silly to first run a bunch of requests in one way and then perform additional request in another way.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a slight difference in "filter" [{"type": "raudikko"}] vs ["raudikko"] but that might not matter

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. I completely forgot about the other file. Yeah, it's probably a good idea to have just one file for requests.

@komu komu merged commit 3c540b0 into EvidentSolutions:master Jan 17, 2023
@komu
Copy link
Member

komu commented Jan 17, 2023

Thanks!

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.

2 participants