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

Update with feedback from @yosifkit #28

Merged
merged 1 commit into from
Sep 24, 2017
Merged

Conversation

wohali
Copy link
Member

@wohali wohali commented Sep 24, 2017

Includes the following feedback from @yosifkit :

  • Permission/ownership changes occur as often as possible in the layer in which the files are created to aid some Docker graph drivers
  • Allow for simplified invocation. Any of the following work to specify additional CLI arguments:
    • docker run couchdb:2.1.0 /opt/couchdb/bin/couchdb +A 16
    • docker run couchdb:2.1.0 couchdb +A 16
    • docker run couchdb:2.1.0 +A 16
    • docker run couchdb:2.1.0 -couch_ini /path/to/my/couch.ini ....
  • gpg --verify replaced with gpg --batch --verify everywhere.

@wohali
Copy link
Member Author

wohali commented Sep 24, 2017

Tests pass; merging.

# grab tini for signal handling
ENV TINI_VERSION v0.16.1
ADD https://github.com/krallin/tini/releases/download/${TINI_VERSION}/tini /usr/local/bin/tini
ADD https://github.com/krallin/tini/releases/download/${TINI_VERSION}/tini.asc /usr/local/bin/tini.asc
Copy link

Choose a reason for hiding this comment

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

Why did this switch to use ADD with a remote URL?
(see https://docs.docker.com/engine/userguide/eng-image/dockerfile_best-practices/#add-or-copy)

There are several problems with this approach. When using ADD in this way, /usr/local/bin/tini has to be downloaded every time docker build is run, regardless of whether it's going to match local build cache. Additionally, this will add double tini to the end image (due to the chmod +x which happens in the following layer).

I'd recommend simply moving this back up into the RUN line which is downloading gosu (where wget is already installed and removed). Also, curl is installed in the first RUN line of this image, so it might be worth either removing that (and adding curl or wget down below in the buildDeps variable) or simply using curl instead when downloading gosu/tini.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was pulled from official documentation for tini.

https://github.com/krallin/tini

Copy link
Member Author

Choose a reason for hiding this comment

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

It sure would be nice if official documentation for projects followed best practices for the communities in which they operate. :( I don't know who to trust at this point.

Copy link

Choose a reason for hiding this comment

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

Doh, that ought to be updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you can give me a block update for tini I'd appreciate it, I'd like to see it separate from gosu just in case we decide to fully remove it in the future (now that it's part of 1.13+ apparently...)

Copy link

Choose a reason for hiding this comment

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

Sure, I can do that -- so, it's technically part of 1.13+ (via docker run --init), but cannot be enabled by default at the image level, and also cannot be enabled for swarm-mode services via CLI (yet).

I'd recommend something similar to https://github.com/tianon/gosu/blob/e87cf95808a7b16208515c49012aa3410bc5bba8/INSTALL.md#from-debian: (which can be shorter if the two are combined, which is why I recommend combining them -- this could also be made shorter by using the curl which is already installed, but I'd really recommend instead removing curl from the image unless CouchDB itself requires it to be installed)

ENV TINI_VERSION 0.16.1
RUN set -ex; \
	\
	apt-get update; \
	apt-get install -y --no-install-recommends wget; \
	rm -rf /var/lib/apt/lists/*; \
	\
	dpkgArch="$(dpkg --print-architecture | awk -F- '{ print $NF }')"; \
	wget -O /usr/local/bin/tini "https://github.com/krallin/tini/releases/download/v${TINI_VERSION}/tini-$dpkgArch"; \
	wget -O /usr/local/bin/tini.asc "https://github.com/krallin/tini/releases/download/v${TINI_VERSION}/tini-$dpkgArch.asc"; \
	\
# verify the signature
	export GNUPGHOME="$(mktemp -d)"; \
	gpg --keyserver ha.pool.sks-keyservers.net --recv-keys 595E85A6B1B4779EA4DAAEC70B588DFF0527A9B7; \
	gpg --batch --verify /usr/local/bin/tini.asc /usr/local/bin/tini; \
	rm -r "$GNUPGHOME" /usr/local/bin/tini.asc; \
	\
	chmod +x /usr/local/bin/tini; \
# verify that the binary works
	tini --version; \
	\
	apt-get purge -y --auto-remove wget

For comparison, here's a simplified version of something similar that keeps the gosu and tini parts separate:

RUN set -ex; \
	\
	apt-get update; \
	apt-get install -y --no-install-recommends wget; \
	rm -rf /var/lib/apt/lists/*; \
	\
	dpkgArch="$(dpkg --print-architecture | awk -F- '{ print $NF }')"; \
	\
# install gosu
	GOSU_VERSION='1.10'; \
	wget -O /usr/local/bin/gosu "https://github.com/tianon/gosu/releases/download/$GOSU_VERSION/gosu-$dpkgArch"; \
	wget -O /usr/local/bin/gosu.asc "https://github.com/tianon/gosu/releases/download/$GOSU_VERSION/gosu-$dpkgArch.asc"; \
	export GNUPGHOME="$(mktemp -d)"; \
	gpg --keyserver ha.pool.sks-keyservers.net --recv-keys B42F6819007F00F88E364FD4036A9C25BF357DD4; \
	gpg --batch --verify /usr/local/bin/gosu.asc /usr/local/bin/gosu; \
	rm -r "$GNUPGHOME" /usr/local/bin/gosu.asc; \
	chmod +x /usr/local/bin/gosu; \
	gosu nobody true; \
	\
# install tini
	TINI_VERSION='0.16.1'; \
	wget -O /usr/local/bin/tini "https://github.com/krallin/tini/releases/download/v${TINI_VERSION}/tini-$dpkgArch"; \
	wget -O /usr/local/bin/tini.asc "https://github.com/krallin/tini/releases/download/v${TINI_VERSION}/tini-$dpkgArch.asc"; \
	export GNUPGHOME="$(mktemp -d)"; \
	gpg --keyserver ha.pool.sks-keyservers.net --recv-keys 595E85A6B1B4779EA4DAAEC70B588DFF0527A9B7; \
	gpg --batch --verify /usr/local/bin/tini.asc /usr/local/bin/tini; \
	rm -r "$GNUPGHOME" /usr/local/bin/tini.asc; \
	chmod +x /usr/local/bin/tini; \
	tini --version; \
	\
	apt-get purge -y --auto-remove wget

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm sorry for the delay here, this got lost because the thread is in a merged PR, not in a new ticket. I'll copy things over to a new issue and get this going again.

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