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

Adds elasticsearch logger #9

Merged
merged 1 commit into from
Mar 21, 2017
Merged

Conversation

techjacker
Copy link
Owner

No description provided.

@techjacker techjacker force-pushed the feature/210-notifications-app branch 3 times, most recently from a526068 to 8976c96 Compare March 20, 2017 16:01
@techjacker
Copy link
Owner Author

Added healthcheck command to Dockerfile

Copy link

@danielepolencic danielepolencic left a comment

Choose a reason for hiding this comment

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

I'd rather NOT use docker-wait. If you can, use the HEALTHCHECK

@techjacker
Copy link
Owner Author

I've added HEALTHCHECK to the Dockerfile.

As far as I can tell doing this merely adds a property to the container metadata (health status), it doesn't delay execution of the container CMD.

So we still need the polling script triggered by docker-compose to get a local development environment set up with docker compose up.

@danielepolencic
Copy link

According to this, it should: docker/compose#4163 (comment)

@techjacker
Copy link
Owner Author

Ok I'll try to get that method working

@danielepolencic
Copy link

Since you have it working, I don't want to stop you merging this PR in.

If you envision this being a sizeable chunk of work, it's better to factor that in a different PR.

@techjacker techjacker force-pushed the feature/210-notifications-app branch from 8976c96 to 132d743 Compare March 21, 2017 12:46
@techjacker
Copy link
Owner Author

reverted docker compose to v2.1 so could remove HEALTHCHECK from the dockerfile (which was failing the CI build as drone is running < docker 1.2)

@techjacker techjacker force-pushed the feature/210-notifications-app branch from 132d743 to 43cf2be Compare March 21, 2017 12:58
@techjacker techjacker force-pushed the feature/210-notifications-app branch from 7dda598 to 5d98786 Compare March 21, 2017 14:59
@techjacker techjacker merged commit 13b8368 into master Mar 21, 2017
@techjacker techjacker deleted the feature/210-notifications-app branch March 21, 2017 15:08
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