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

feat(ci): add docker image builds per PR #1881

Merged
merged 1 commit into from
Aug 9, 2023
Merged

Conversation

vpavlin
Copy link
Member

@vpavlin vpavlin commented Aug 3, 2023

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

  • additional Dockerfile under docker/binaries/ (following nimbus folder structure here)
  • additional job to build wakunode2, build the container image, push it to registry and provide comment

Issue

related to #1877

Copy link
Contributor

@jm-clius jm-clius left a 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.

@vpavlin vpavlin force-pushed the feat/docker-image-pr branch 2 times, most recently from 2f42ccc to 44bf4cd Compare August 8, 2023 12:40
@github-actions
Copy link

github-actions bot commented Aug 8, 2023

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:1881

@vpavlin
Copy link
Member Author

vpavlin commented Aug 8, 2023

Ha! It actually works!

Copy link
Collaborator

@Ivansete-status Ivansete-status left a 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
Copy link
Collaborator

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?

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 can add a comment there - something like "A Dockerfile to build a distributable container image from pre-existing binaries" ??

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes that sounds great!


LABEL maintainer="vaclav@status.im"
LABEL source="https://github.com/waku-org/nwaku"
LABEL description="Wakunode: Waku and Whisper client"
Copy link
Collaborator

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" :)

Copy link
Member Author

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

@vpavlin
Copy link
Member Author

vpavlin commented Aug 9, 2023

Is it possible to use the existing Dockerfile? I have the impression that we are adding extra complexity :)

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:)

@github-actions
Copy link

github-actions bot commented Aug 9, 2023

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:1881

@Ivansete-status
Copy link
Collaborator

Is it possible to use the existing Dockerfile? I have the impression that we are adding extra complexity :)

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:)

Lovely, thanks for the answer!

@vpavlin vpavlin merged commit 84f94d5 into master Aug 9, 2023
12 checks passed
@vpavlin vpavlin deleted the feat/docker-image-pr branch August 9, 2023 11:13
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.

None yet

3 participants