-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
Resolves "Alpine version of container #32" #40
Resolves "Alpine version of container #32" #40
Conversation
404f8f9
to
ca57a86
Compare
@@ -0,0 +1,15 @@ | |||
# Only alpine version that suports rethinkdb is edge | |||
FROM alpine:edge | |||
|
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.
Needs a maintainer line. Right now, all Dockerfiles list @dalanmiller as maintainer (example: https://github.com/rethinkdb/rethinkdb-dockerfiles/blob/master/jessie/2.3.6/Dockerfile#L3), but, per docker-library/official-images#3332 (comment), we should probably change these to @AtnNn. (For now, I'd stick with copying the @dalanmiller credit, to make one-shot updating everything simpler.)
# Only alpine version that suports rethinkdb is edge | ||
FROM alpine:edge | ||
|
||
ENV RETHINKDB_VERSION 2.3.6-r1 |
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.
For consistency and compatibility with the images for other distros, this should be RETHINKDB_PACKAGE_VERSION
, as it is in the Debian dockerfiles (see https://github.com/rethinkdb/rethinkdb-dockerfiles/blob/master/jessie/2.3.6/Dockerfile#L10 for an example).
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.
Any reason why you want to pin a version? Current release on Alpine 3.7 is -r4
|
||
WORKDIR /data | ||
|
||
EXPOSE 28015 29015 8080 |
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.
I'd prefer it if the EXPOSE
directive were moved to come after CMD
, with the comment stating which ports are which, the way it is in the other Dockerfiles.
@@ -0,0 +1,15 @@ | |||
# Only alpine version that suports rethinkdb is edge | |||
FROM alpine:edge |
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.
If there's no stable version of Alpine that supports RethinkDB, then we probably shouldn't be packaging for it yet (at least, not as the new base for the official image). The few Dockerfiles in the official library that are packaged for Alpine (by a cursory sampling of images I'd expect to see migrated, it would seem that the announcement of increased forward adoption of Alpine a year and a half ago has been greatly exaggerated) don't do it without pinning a specific release, and edge
is considered unsuitable for production.
Alternately, this could be listed as a Dockerfile for Alpine Edge, but it would need to go in a subdirectory for Alpine release versions (just as the repo is currently divided into separate directories for different Debian and Ubuntu releases). If doing this, I'd introduce a top-level distro directory level to the hierarchy (ie. moving the repo's jessie
directory to debian/jessie
) - I skipped this when originally creating the repo because, at the time I originally devised the current hierarchy, the intersection of "distros with official Docker images" and "distros that RethinkDB upstream builds packages for" was exclusively limited to the Debian / Ubuntu non-conflicting-named-release family.
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.
Actually Alpine 3.7 stable ships RethinkDB, no need for edge.
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.
As I noted 26 days ago, Alpine's actually been packaging RethinkDB since 3.5.
@stuartpb Could you speak up in this thread: docker-library/official-images#3332 |
I don't know the current status of the upstream RethinkDB maintainership: I've got a PR in the works that'll raise this question directly. |
To answer my question about Edge above, it looks like Alpine upstream's been packaging this since 3.5 in April: https://pkgs.alpinelinux.org/packages?name=rethinkdb On that basis, I'll probably push a few PRs that addresses the points I raised in my review above in a little bit:
|
# Only alpine version that suports rethinkdb is edge | ||
FROM alpine:edge | ||
|
||
ENV RETHINKDB_VERSION 2.3.6-r1 |
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.
Any reason why you want to pin a version? Current release on Alpine 3.7 is -r4
@mterron both Alpine latest and 3.7 now include rethinkdb package by default so it can be installed directly now from alpine repo https://pkgs.alpinelinux.org/packages?name=rethinkdb&branch=v3.7 |
I'd recommend building from scratch instead of installing a package that limits you to the version available in the OS repos: |
@daveisfera I tried your script (a few months ago) and it fails to build an image with rethinkdb 2.3.6 but with 2.3.5 it works nice. meanwhile, with the alpine package, I can successfully run 2.3.6 |
This commit added |
@daveisfera yet the one from the package manager work. trust me, I agree with your approach, but some people just want an image that works. |
That works until 2.4 comes out or 2.3.7 is released and the Alpine package isn't updated. I'm guessing that the Alpine package built because it uses the original 2.3.6 version and not the one with the 2.3.6-windows version with the problematic commit that I mentioned. I opened a RethinkDB bug because I believe that's going to cause problems with future Alpine builds as well. |
Here's a patch to workaround that issue:
And then a
|
Apparently, some of the files take a lot of memory to compile and when I use more than 1 cpu on my laptop, it fails when it runs out of memory, so I'd take out the |
Here's a version that keeps the patch local instead of downloading it from the Alpine packaging repo:
I also submitted a PR with the LibreSSL patch ( rethinkdb/rethinkdb#6671 ) and there's a link to the patch there. Once that's merged, it would no longer be needed in the |
Closing in favor of #45. |
Creates Dockerfile for an Alpine version of RethinkDB.
The result image is almost 4x smaller then original RethinkDB image:
resolve #32