-
Notifications
You must be signed in to change notification settings - Fork 26
Added docker build and quick-start #101
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
base: main
Are you sure you want to change the base?
Added docker build and quick-start #101
Conversation
I updated #40 with how I think the "official" docker container should be implemented. However, I really like the idea of what is presented here to use docker as a developer tool for building and testing. I've also responded to some of your questions here #2 (comment) |
@tonygermano, I'm no longer aware of issues outside of the registry name (presumably someone owns the official URL?). Differences I'm aware of relative to the original repo:
Happy to discuss as needed. Every compose file, kubernetes deployment, etc... that I've tested doesn't notice the difference between this and stock. |
@mgaffigan I pulled your launcher script out of this PR, made significant changes, and put it in its own PR in #119 . I think it is useful outside of a docker context, and it can still be used with docker. Would you mind reviewing it and giving any feedback? |
@tonygermano, Excellent! I'd love to see that merged and agree it is useful outside of docker. |
COPY .sdkmanrc . | ||
RUN apt-get update\ | ||
&& apt-get install -y zip curl\ | ||
&& curl -s "https://get.sdkman.io?ci=true" | bash \ |
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.
Why do we need SDKMan and not just an ubuntu jdk image?
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 think either is acceptable, and avoiding sdkman would certainly make the dockerfile simpler. I think I went this route because the zulu jdk image does not contain ant, so I had to pull additional packages regardless.
I'll see what that would look like.
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.
See mgaffigan@123bbe2 for changes to avoid sdkman. Problems are:
- Zulu nor Temurin seem to publish a JDK with FX included. Install from deb or tgz requires quite a few deps.
- Ant requires separate installation
- Docker revisions can get out of sync with
.sdkmanrc
I don't love sdkman, but it does seem simpler to use it in the builder container.
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 agree with the opinion that neither of the two options are perfect. Using a jdk image can easily drift with our preferred version from .sdkmanrc
, and installing sdkman in a container is just nasty.
But in this case installing sdkman in the container is the better way to go to ensure as identical as possible envs.
Zulu nor Temurin seem to publish a JDK with FX included. Install from deb or tgz requires quite a few deps.
I'm not sure this is true. the 8.0.442.fx-zulu
specified in .sdkmanrc
is one example of that. Unless you're talking about Docker images.
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'm referring to docker images. Azul and Temurin publish tgz of JavaFX, but they do not provide pre-built images. See mgaffigan@123bbe2 for the changes to install from tgz rather than sdkman.
…scripts) Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
a3709ea
to
6b7246f
Compare
Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
I've updated the PR to depend upon #119 for oieserver.sh and match the conventions of #126 assuming that has more community input. Currently blocked by merge of: #119 and #126 Process used for testing:
|
Don't link to #126, it is still a draft and not ready for review. |
.dockerignore
Outdated
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.
suggestion: I think having an "ignore everything" and "unignore this" approach would be more readable here
*
!setup/
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.
Since we are building from source, I think an exclude list would be substantially more manageable than an include 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.
When writing the previous comment, I was thinking of the release container.
But, if we're talking about a build container for development, I still think includelisting the 6 directories + possibly .sdkmanrc
might be the cleaner way
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.
Can you help me understand what you are intending? The primary goal of a .dockerignore
is to avoid copying build artifacts, not to select things for the docker build. It should generally match .gitignore (since if you are not putting it in code-control, it probably should not be used for build).
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.
The way I see it is that we have two options to use containers in the development cycle.
- Compile OIE natively (
ant build
) from the host machine and package the build result into a container for runtime testing - Compile OIE inside a container that already packages required dependencies (and continue building runtime images from this)
With both approaches the build context should be as small as possible.
With approach 1 the context should only contain the setup/
directory and possibly some individual files. This is what I intended in the first comment.
With approach 2 things get a little more complicated, as we might need most of the dirs + files in the repo par some license or instructions files. But it is quite easy to find us with a bunch of little files that tweak some tooling or another, but are not necessary to compile OIE, which is why in my mind whitelisting only the files + dirs required for compilation might be the simpler way to go.
@@ -0,0 +1,98 @@ | |||
# syntax=docker/dockerfile:1.7-labs |
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.
suggestion: I think we should stay on docker/dockerfile:1
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.
docker/dockerfile:1
doesn't support COPY --exclude=...
and would thus result in increased image sizes.
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.
question: Could the --exclude
directives be run as rm
-s in the builder
stage?
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.
You cannot run an rm
in the same layer as a COPY, so the image size would still be inflated. I think the alternative here would be to add an option to mirth-server.xml
to not copy them to setup
.
The other options I'm disagreeing with are:
- Copy each item from setup by name (verbose, fragile if new item added)
- Copy to a third location (symlink?), then copy to final (ew for complexity)
- Take the hit on layer-size (48 MB)
- ADD with special tar (ew for time/space and general disgust)
I generally agree with the advice to stay on docker/dockerfile:1
, but --exclude
is fairly well supported by this point, and should get taken out of labs soon.
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.
Out of curiosity since I don't currently have the means to test this, but when doing a multi-stage build with a COPY --from
, will the layers from the first stage be included in the second stage or is there any squashing action going on?
Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
COPY .sdkmanrc . | ||
RUN apt-get update\ | ||
&& apt-get install -y zip curl\ | ||
&& curl -s "https://get.sdkman.io?ci=true" | bash \ |
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 agree with the opinion that neither of the two options are perfect. Using a jdk image can easily drift with our preferred version from .sdkmanrc
, and installing sdkman in a container is just nasty.
But in this case installing sdkman in the container is the better way to go to ensure as identical as possible envs.
Zulu nor Temurin seem to publish a JDK with FX included. Install from deb or tgz requires quite a few deps.
I'm not sure this is true. the 8.0.442.fx-zulu
specified in .sdkmanrc
is one example of that. Unless you're talking about Docker images.
########################################## | ||
# | ||
# Ubuntu JDK Image | ||
# | ||
########################################## | ||
|
||
FROM eclipse-temurin:21.0.7_6-jdk-noble as jdk-run | ||
|
||
RUN groupadd engine \ | ||
&& usermod -l engine ubuntu \ | ||
&& adduser engine engine \ | ||
&& mkdir -p /opt/engine/appdata \ | ||
&& chown -R engine:engine /opt/engine | ||
|
||
WORKDIR /opt/engine | ||
COPY --chown=engine:engine --from=builder \ | ||
--exclude=cli-lib \ | ||
--exclude=mirth-cli-launcher.jar \ | ||
--exclude=mccommand \ | ||
--exclude=manager-lib \ | ||
--exclude=mirth-manager-launcher.jar \ | ||
--exclude=mcmanager \ | ||
/app/server/setup ./ | ||
|
||
VOLUME /opt/engine/appdata | ||
VOLUME /opt/engine/custom-extensions | ||
EXPOSE 8443 | ||
|
||
USER engine | ||
ENTRYPOINT ["./configure-from-env.sh"] | ||
CMD ["./oieserver"] | ||
|
||
########################################## | ||
# | ||
# Alpine JRE Image | ||
# | ||
########################################## | ||
|
||
FROM eclipse-temurin:21.0.7_6-jre-alpine as jre-run | ||
|
||
# Alpine does not include bash by default, so we install it | ||
RUN apk add --no-cache bash | ||
# useradd and groupadd are not available in Alpine | ||
RUN addgroup -S engine \ | ||
&& adduser -S -g engine engine \ | ||
&& mkdir -p /opt/engine/appdata \ | ||
&& chown -R engine:engine /opt/engine | ||
|
||
WORKDIR /opt/engine | ||
COPY --chown=engine:engine --from=builder \ | ||
--exclude=cli-lib \ | ||
--exclude=mirth-cli-launcher.jar \ | ||
--exclude=mccommand \ | ||
--exclude=manager-lib \ | ||
--exclude=mirth-manager-launcher.jar \ | ||
--exclude=mcmanager \ | ||
/app/server/setup ./ | ||
|
||
VOLUME /opt/engine/appdata | ||
VOLUME /opt/engine/custom-extensions | ||
|
||
EXPOSE 8443 | ||
|
||
USER engine | ||
ENTRYPOINT ["./configure-from-env.sh"] | ||
CMD ["./oieserver"] |
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.
question: These images have been moved to engine-docker
and will be built with OpenIntegrationEngine/engine-docker#1. Would it be reasonable to edit the Dockerfile
in this repo to be a build/local container only?
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.
Can you clarify what you are suggesting be removed? I think it is useful to be able to do a local build of ubuntu or alpine, since those are the release environments and would thus both be needed for testing.
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.
issue: @OpenIntegrationEngine/maintainers correct me if I'm wrong, but this file might live in engine-docker repo instead. I don't have a good suggestion on how to sync the two.
COPY .sdkmanrc . | ||
RUN apt-get update\ | ||
&& apt-get install -y zip curl\ | ||
&& curl -s "https://get.sdkman.io?ci=true" | bash \ |
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.
nitpick: Downloading and executing arbitrary scripts is not good practice. The correct way should be to download the binaries from GH and set them up ourselves, but it quite big hassle and I've never completed it, so might be good enough for now.
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'm not sure I see the difference between using sdkman and installing sdkman - since they both download and run untrusted scripts.
If we were aiming for zero-trust, then we would need to shift to installing from tgz and add hash verifications. sdkman is fundamentally opposed to that. Practically, I would recommend separately maintaining and publishing a javafx container then basing this file on that.
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.
In my mind the difference is that we have no control over the script being downloaded from https://get.sdkman.io
. It carries the risks of DNS poisoning (although highly unlikely), and malicious changes by sdkman maintainers themselves - also highly unlikely given their size. But then again, xz
was even larger.
What I'm saying is this is something we should keep in mind. I don't have a viable alternative right now.
@@ -0,0 +1,98 @@ | |||
# syntax=docker/dockerfile:1.7-labs |
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.
question: Could the --exclude
directives be run as rm
-s in the builder
stage?
While this might be somewhat responsive to #40, it is primarily just to provide a quick-start way for a developer to build and test. While trying to address Issue 5 I had hours of challenges getting things building happily - and shifted to docker as an escape hatch.
I do not expect this PR is ready to merge as is, but wanted to get something posted publicly. Known issues include:
That said, it does work to the point of passing tests, running server, client, and simple message processing.
I'm new to the project, so if I've missed a step please let me know.