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: Build Docker Arm64 images #448

Merged
merged 1 commit into from
Feb 23, 2023
Merged

Conversation

kkopachev
Copy link
Contributor

Adding support for building arm64 docker image in the same manifest.
Also removing docker build step as results are not used and with release image rebuild anyway. I'd argue that the fact that it builds outside of docker means it will build inside of it and there is no reason to break the build, but I can go either way.

Here is a build on my repo

Copy link
Contributor

@stepanstipl stepanstipl left a comment

Choose a reason for hiding this comment

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

First thanks, I think this is a great addition 👍 🎉

A couple of minor notes to clean up some bits which I believe are not necessary - I've commented directly in the code.

Also removing docker build step as results are not used and with release image rebuild anyway. I'd argue that the fact that it builds outside of docker means it will build inside of it and there is no reason to break the build, but I can go either way.

I would really prefer to keep this. The reason is that we're testing different things - i.e. mainly the Dockerfile/image itself.

For example - should the Dockerfile have a wrong syntax, we would only find out in the Push Image job, which runs only after the release has been created, and that is too late. I would actually suggest extending the build-docker to also cover the newly added platform.

Having said that - and I think this is not much of an issue, as the jobs run in parallel - but there's certainly some space for optimisation to avoid building the docker image(s) twice. However, it's a bit of catch 22 - it's IMHO important to test and build everything before publishing/uploading release and images, simply to confirm it works. Perhaps it would be possible to reuse the already-built images? Happy to accept PRs on that :). In any case - let's leave it for potential separate PR.

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@kkopachev
Copy link
Contributor Author

kkopachev commented Feb 23, 2023

I brought back separate build images step and make it so it exports layers to Github Actions cache, then push image job should be able to use cache and skip the build entirely.

I see your point about checking that image builds. I hope GHA cache could save some build time, although it only takes 3 minutes anyway.
I guess ideal solution would be to use docker bake to build and test everything from within docker image. There is a way to build binaries following build steps in Dockerfile. Here is an example for building cross-compiled binaries from Dockerfile through docker bake. Although, I think that would be too much for current PR.
Or there is another way - copy pre-built binaries into docker image like it is done in opa

@stepanstipl stepanstipl changed the title Build Docker Arm64 images feat: Build Docker Arm64 images Feb 23, 2023
@stepanstipl stepanstipl self-assigned this Feb 23, 2023
@stepanstipl stepanstipl added the feature New feature or request label Feb 23, 2023
Copy link
Contributor

@stepanstipl stepanstipl left a comment

Choose a reason for hiding this comment

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

Thanks @kkopachev, for clarifying the platform bits and the other changes. I also really like the GHA cache improvement 👍 .

I wasn't familiar with the bake command, but seems interesting. My overall wish for the CI/CD/build parts is to make them more platform-independent and fully reproducible locally. I'm working on another project - so far private - to perhaps move most of the CI/CD/build tooling outside to a separate project (well-tested, reusable) which would in turn allow focusing more on the functional side here. But that's more of a future wish for now ;), for sure better left for other PRs.

@stepanstipl stepanstipl merged commit 1d31e54 into doitintl:master Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants