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 wait-for-it.sh to Docker image #291

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add wait-for-it.sh to Docker image #291

wants to merge 1 commit into from

Conversation

straurob
Copy link

@straurob straurob commented May 9, 2020

Motivation

When running both, MailHog and MongoDB, in a Docker environment, it's not unlikely that the MailHog container is up and running before the MongoDB container. In this case MailHog will use its in-memory storage which probably is not what you want.

Description

This PR adds wait-for-it.sh from https://github.com/vishnubob/wait-for-it to the Docker image. This enables you to override the default MailHog entrypoint with wait-for-it.sh as shown below in an example Docker Compose file.

docker-compose.yml

services:
  mailhog:
    entrypoint: ["wait-for-it.sh", "mongo-db-host:27017", "--strict", "--timeout=120", "--", "MailHog"]

This example waits 120s until a connection to the MongoDB can be established before starting MailHog.

@omeid
Copy link

omeid commented May 21, 2020

I would suggest considering docker compose dependencies with health checks.

@straurob
Copy link
Author

I would suggest considering docker compose dependencies with health checks.

This was my initial thought as well. As far as I understand the Docker documentation the recommended approach is to use wait-for-it or similar.

The support for the conditional depends_on is not available since Docker compose version 3.

Even when using depends_on in combination with a restart on-failure does not seem to work because MailHog just performs a fallback to its in-memory storage if the MongoDB connection is not available.

Or am I missing something else?

@omeid
Copy link

omeid commented May 22, 2020

docker-compose schema version 2 is still supported and many people use it, the up coming Compose Spec will most likely support the depends and service_healthy conditions due to this widespread use.

I think it should work fine with restarts as well.

@thaJeztah
Copy link

I don't think depends_on in a compose file is a full replacement for a container being designed for fault-tolerance. docker-compose (as a client side tool) does not perform a reconciliation loop for the whole container lifecycle, so if the dependent service becomes unavailable (e.g. mongoldb crashes), the depends_on won't resolve that situation.

ideally Mailhog itself would have an (configurable) option try reconnecting after a failure (and disable fallback to the in-memory-store), but an entrypoint script could be an alternative to that (at least for the initial startup)

@straurob
Copy link
Author

ideally Mailhog itself would have an (configurable) option try reconnecting after a failure (and disable fallback to the in-memory-store), but an entrypoint script could be an alternative to that (at least for the initial startup)

I tried implementing your idea in PR 17 of the MailHog-Server repo: mailhog/MailHog-Server#17

@bryanvaz
Copy link

Agree with @straurob and @thaJeztah, though the main underlying issue has to do with deterministic behaviour within development and test environments.

Essentially, Mailhog, when running in a docker environment, should not silently change databases based on whether or not another service is running at boot - or at least there should be an option to disable the failover function and manually specify a connection type.

Without this kind of control, I would not be surprised if unsuspecting users would create test runs that fail randomly because mongo failed to start up in time, or a developer's test emails all disappear because just because they restarted the stack (even though they were using mongo for persistence).

@Lewiscowles1986
Copy link

I had a much simpler take on all this. Mailhog is a golang binary. Needing to deploy a shell-script with it seems contrary to the goals of most golang binaries. Would it be possible to add a second command to mailhog binary, so that the same binary can be used?

That then would raise some further questions:

  • Does the command wait for persistence (looping, probably with timeout)?
  • Does the command simply check for persistence and fail if it is not present?
  • What guidance do deployers need for such a command (I worked at places where they have this check and then health check every second that a database is connectable... Its not good and you wind up with unnecessary load from health check BS)

@@ -19,6 +21,10 @@ RUN apk --no-cache add --virtual build-dependencies \
# https://github.com/boot2docker/boot2docker/issues/581
RUN adduser -D -u 1000 mailhog

# Download wait-for-it.sh
RUN wget https://raw.githubusercontent.com/vishnubob/wait-for-it/master/wait-for-it.sh \

Choose a reason for hiding this comment

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

This should pin to a specific version to make it deterministic; as that repository hasn't tagged releases, you could pin to the commit currently matching "master";

RUN wget https://raw.githubusercontent.com/vishnubob/wait-for-it/81b1373f17855a4dc21156cfe1694c31d7d1792e/wait-for-it.sh \

And this should be in a separate stage, because wget is only needed to download the script, and should not be in the final image, as it's not needed at runtime. This PR also needs a rebase, as it's based on an outdated version of the Dockerfile (which was rewritten to a multi-stage build in 9c830be (#317))

#
# MailHog Dockerfile
#

FROM alpine AS waitforit
RUN apk --no-cache add wget
RUN wget https://raw.githubusercontent.com/vishnubob/wait-for-it/81b1373f17855a4dc21156cfe1694c31d7d1792e/wait-for-it.sh \
 && chmod a+x wait-for-it.sh


FROM golang:alpine as builder

# Install MailHog:
RUN apk --no-cache add --virtual build-dependencies \
git \
&& mkdir -p /root/gocode \
&& export GOPATH=/root/gocode \
&& go get github.com/mailhog/MailHog

FROM alpine:3
RUN apk --no-cache add bash
# Add mailhog user/group with uid/gid 1000.
# This is a workaround for boot2docker issue #581, see
# https://github.com/boot2docker/boot2docker/issues/581
RUN adduser -D -u 1000 mailhog

COPY --from=waitforit /wait-for-it.sh  /
COPY --from=builder /root/gocode/bin/MailHog /usr/local/bin/

USER mailhog

WORKDIR /home/mailhog

ENTRYPOINT ["MailHog"]

# Expose the SMTP and HTTP ports:
EXPOSE 1025 8025

however having to add bash just for this script seems like a bit too much; perhaps instead a more minimal script should be written that does just the things that this image needs or, as I mentioned in an earlier comment, MailHog itself should really have an option to do the retries.

Altogether, I'm not sure if this script should really be part of the default image, as it's not used by default (especially with the added bash dependency)

@thaJeztah
Copy link

Mailhog is a golang binary. Needing to deploy a shell-script with it seems contrary to the goals of most golang binaries. Would it be possible to add a second command to mailhog binary, so that the same binary can be used?

I agree; #291 (comment)

ideally Mailhog itself would have an (configurable) option try reconnecting after a failure (and disable fallback to the in-memory-store) ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants