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

Docker compose #18

Merged
merged 3 commits into from
Dec 21, 2020
Merged

Docker compose #18

merged 3 commits into from
Dec 21, 2020

Conversation

Ben-Haslam
Copy link
Contributor

No description provided.

@Ben-Haslam Ben-Haslam requested a review from flofrie December 21, 2020 12:22
Copy link
Collaborator

@flofrie flofrie left a comment

Choose a reason for hiding this comment

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

I just had a look at the dockerize documentation. Neat tool. Still, I am surprised there isn't some docker-native way to wait for some service to come up before starting something else. But no matter, for now, this clearly does the job. -- Found an interesting link in the documentation to discussion of this: docker/compose#374 (comment)

@ben-clearmatics , for clarification: neither the ENTRYPOINT directive, nor main.sh get changed here to actually use dockerize. You're literally just making sure dockerize exists within the container. I guess that's as intended, and for the purposes of our docker compose deployment strategy, another image is derived from this that then makes use of dockerize?

BTW, generally a word of caution to all of us: this is how at some point down the line, really significant hacks can happen. We're including here in a container running our nodes a tool from some guy's Git repo, most of which is 4 year old code, and we have no actual audit or knowledge what his code does, and don't really know what happens if he changes anything. (Or, if someone with access to his Github account changes anything ...) -- Over time, we must become more aware of the many times we're doing something like this, and think hard about solutions.

@Ben-Haslam
Copy link
Contributor Author

I just had a look at the dockerize documentation. Neat tool. Still, I am surprised there isn't some docker-native way to wait for some service to come up before starting something else. But no matter, for now, this clearly does the job. -- Found an interesting link in the documentation to discussion of this: docker/compose#374 (comment)

@ben-clearmatics , for clarification: neither the ENTRYPOINT directive, nor main.sh get changed here to actually use dockerize. You're literally just making sure dockerize exists within the container. I guess that's as intended, and for the purposes of our docker compose deployment strategy, another image is derived from this that then makes use of dockerize?

BTW, generally a word of caution to all of us: this is how at some point down the line, really significant hacks can happen. We're including here in a container running our nodes a tool from some guy's Git repo, most of which is 4 year old code, and we have no actual audit or knowledge what his code does, and don't really know what happens if he changes anything. (Or, if someone with access to his Github account changes anything ...) -- Over time, we must become more aware of the many times we're doing something like this, and think hard about solutions.

Yes, all that is changed is the dockerfile. I agree that its not great it uses a repo that is not ours, but its the only solution I could find for this problem. A lot of people have asked for them to update compose to be able to do this natively so that may come soon. When compose runs this image it modifies the entrypoint, such that it uses dockerize to wait until the fluentd container is up and running before starting the container, so the container needs to have dockerize installed for that to work.

@Ben-Haslam Ben-Haslam merged commit bbfd44a into master Dec 21, 2020
@Ben-Haslam Ben-Haslam deleted the docker-compose branch January 6, 2021 16:22
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