-
Notifications
You must be signed in to change notification settings - Fork 54
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
feat(ci): add docker image builds per PR #1881
Conversation
8acfd4a
to
fe19ef4
Compare
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.
LGTM and this adds a valuable extra layer of CI. Wondering though if we shouldn't start adding short description of each CI job and their purpose in our README?
to get the image served as quickly as possible, there is an extra wakunode2 build running
Mmm. I don't see this as a major issue, at least for now. Assuming we'll often see both failing, which is a bit of wasted effort, but at least we test the docker image building process from start to finish.
2f42ccc
to
44bf4cd
Compare
You can find the image built from this PR at
|
Ha! It actually works! |
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.
Thanks for that! This is awesome!
Is it possible to use the existing Dockerfile? I have the impression that we are adding extra complexity :)
@@ -0,0 +1,33 @@ | |||
FROM debian:stable-slim as prod |
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.
Shall we add a comment at the top indicating the expected purpose of this Dockerfile?
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 can add a comment there - something like "A Dockerfile to build a distributable container image from pre-existing binaries" ??
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 that sounds great!
docker/binaries/Dockerfile.bn.amd64
Outdated
|
||
LABEL maintainer="vaclav@status.im" | ||
LABEL source="https://github.com/waku-org/nwaku" | ||
LABEL description="Wakunode: Waku and Whisper client" |
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 never realized that but maybe we should avoid mentioning "Whisper client" :)
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.
Yeah, I was thinking about that, but kept it to match our current Dockefile:D
We could, but I would like to gradually move away from rebuilding the binary in the docker build and rather just import some existing binary. We will still use Docker to build the binaries, but the goal is "build once, distribute in many ways". This is a first step where we introduce (and test) a Dockerfile which can take an existing binary and only use the Docker build as a way to produce a distributable package. Hope it makes sense:) |
44bf4cd
to
b78488b
Compare
b78488b
to
a1918a3
Compare
You can find the image built from this PR at
|
Lovely, thanks for the answer! |
Description
This PR adds a separate Dockerfile which creates an image from a binary built in CI.
It also adds a job which pushes it to a Docker repository (quay.io/wakuorg/nwaku-pr) separate from the one where standard releases are pushed (to keep the latter clean of these "ephemeral" images) - the PR images are cleaned up after 7 days.
Additional step comments the PR to present the newly created image to contributor and reviewers.
You can see the example here: nwaku-test-org#12
There is one caveat - to get the image served as quickly as possible, there is an extra
wakunode2
build running, this is not ideal, but it would increase "interactivity" of the PR. It is also easy to just "append" these 2 steps to existing build job if that is preferred - interested in thoughts.Another option would be to split the build job and first build
wakunode2
and the image and then the rest of the binaries (other apps and tools), which are not used any further anyway.Changes
docker/binaries/
(following nimbus folder structure here)wakunode2
, build the container image, push it to registry and provide commentIssue
related to #1877