-
Notifications
You must be signed in to change notification settings - Fork 137
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
Introduce a CI/CD-friendly Dockerfile #59
Conversation
One detail I realized while chatting with @rnewson and @chewbranca on IRC about this is that the source code is removed entirely from the final image because it lives in one of the build layers, so that messes up the use case where one could imagine a developer doing |
Thanks @kocolosk , I will look at these later in the week. |
Cool take your time, half of this I only learned about in the last 36 hours so it’s entirely possible that this is a moving target. Eager for any feedback towards that end. |
dev-cluster/Dockerfile
Outdated
texlive-fonts-recommended \ | ||
texlive-latex-extra \ | ||
&& curl -s https://deb.nodesource.com/gpgkey/nodesource.gpg.key | apt-key add - \ | ||
&& echo 'deb https://deb.nodesource.com/node_6.x jessie main' > /etc/apt/sources.list.d/nodesource.list \ |
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.
Node 6 is end of life now. Could you update this to use node 8 which is the current LTS.
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.
Certainly. That was a straight copy/paste from the old Dockerfile but happy to upgrade it as part of this PR.
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.
@garrensmith The README in Fauxton still says we require "at least Node 6 and npm 3", is that no longer true? If not we'll need to change the build for non-dev instances, too.
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.
Its fine to use node 6. I recall you asking me to try to use node 6 for as long as possible so I have intentionally made sure it still works on 6. But since this is a dev image it would be better if we were using a more recent version of Node.
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.
@garrensmith out of curiosity where do I go to find the node release cycle? I tried https://github.com/nodejs/Release but I couldn't match it up with your comment here.
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.
@kocolosk my guess is that he means it moves to maintenance LTS in a couple of months, i.e. no more improvements other than security patches...and it's probably fair to say that it's not getting a lot of actual changes these days, either... ;)
dev/Dockerfile
Outdated
|
||
MAINTAINER CouchDB Developers [email protected] | ||
LABEL maintainer="CouchDB Developers <[email protected]>" |
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.
Is this the new format? If so, can you issue a separate PR to improve this in the other Dockerfiles? Thanks! :)
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, MAINTAINER is listed as deprecated in the Dockerfile docs:
https://docs.docker.com/engine/reference/builder/#maintainer-deprecated
Will file another PR, thanks.
Couple of points:
|
Hah, yeah, that was an impressive code review! I will be sure to double-check if there are additional PRs to file against the 2.1.1 image. |
@daleharvey Are you still the right person to ping on this for PouchDB? Are you currently using the couchdb dev Docker image? If so, will @kocolosk 's changes above cause you any heartburn? |
After much thought, I've decided that changing what Since I know you get busy @kocolosk I can finish the PR if you like, would like to get this merged for 2.2.0. |
This paves the way for a simpler dev image Dockerfile.
This dev image configuration is modeled after the 2.1.1 Dockerfile with a few modifications useful for day-to-day development: * The Dockerfile builds from git rather than the official source releases. The build is configurable using the following build_args: clone_url (default: https://gitbox.apache.org/repos/asf/couchdb.git) checkout_branch (default: master) configure_options (default: <blank>) The configure_options are passed directly to ./configure and can be used to e.g. --disable-docs or --disable-fauxton: docker build --build-arg checkout_branch=my-cool-feature dev/ * We take advantage of multi-stage builds [1] to create a series of layers that optimize build time without inflating the final image size. In normal development the layers that install runtime and build dependencies will be cached, and the build will start by updating and configuring the existing git clone. This work includes the changes proposed in #50 and #57.
5b2cea9
to
2ecab90
Compare
2ecab90
to
4ae5ad7
Compare
I have taken the liberty of fixing the I'll bake these changes here for a bit, and if they go well, use them to build the 2.2 images once CouchDB releases soon. |
Thanks @wohali this all looks great! |
This dev image configuration is modeled after the 2.1.1 Dockerfile with a few modifications useful for day-to-day development:
build-args
:The configure_options are passed directly to ./configure and can be used to e.g.
--disable-docs
or--disable-fauxton
:This work includes the changes proposed in #50 and #57. It moves the existing "cluster-in-a-box" Dockerfile to
dev-cluster
; I'd be curious to see if anyone uses that approach. Personally I think we should continue to refine and harden the Helm chart providing a Kubernetes StatefulSet and refer folks there instead.