-
Notifications
You must be signed in to change notification settings - Fork 17
Added Docker support #65
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, added a small question
| build-docker: | ||
| docker build -t connectors . | ||
|
|
||
| run-docker: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it depend on build-docker? If so, should we also declare the dependency and build the image if it's not built?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes and no. if you run it without the image built, it'll fail. But it should run without re-buidling everytime. I can't think of a simple mechanism to trigger the build if it was not done when running
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, I was also on the fence whether it should be a hard requirement. So if the docker image was never built, we still get a somewhat meaningful error message, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
% docker run --rm -it -p 127.0.0.1:9292:9292/tcp connectors
Unable to find image 'connectors:latest' locally
docker: Error response from daemon: pull access denied for connectors, repository does not exist or may require 'docker login': denied: requested access to the resource is denied.
See 'docker run --help'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm late to the party, but small comment.
| docker build -t connectors . | ||
|
|
||
| run-docker: | ||
| docker run --rm -it -p 127.0.0.1:9292:9292/tcp connectors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hardcodes the port, which may not be in alignment with connectors.yml Is that something we need to worry about? Should we also be sure that the docker image can be run with env-vars which overwrite your yaml configs, like we do for our other docker images?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you're right, we should not hardcore the port. For the env it sounds like a great improvement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed an issue for this: https://github.com/elastic/enterprise-search-team/issues/1765
Adds a Dockerfile that installs and deploys the app using Ubuntu Focal
Also Fixes #64