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

Add arm64 support to Docker images #1541

Merged
merged 5 commits into from
May 20, 2022
Merged

Add arm64 support to Docker images #1541

merged 5 commits into from
May 20, 2022

Conversation

niccoloraspa
Copy link
Member

@niccoloraspa niccoloraspa commented May 19, 2022

Closes: #XXX

What is the purpose of the change

This PR extends the work done in #1535 to introduce support for arm64 architecture making osmosis docker image multi-architecture (amd64 and arm64).

I have also updated the CI to build and push the image for multiple architectures.

Please note that it takes ~20 minutes for the build complete but it would run only on every new tag so I think it's acceptable.

Brief Changelog

  • Modify Dockerfile to add arm64 support
  • Update docker CI to build and push for arm64

Testing and Verifying

You can build for arm64 with:

docker buildx build --platform linux/arm64 --tag osmosis:arm64 .

I tested the CI in my fork: https://github.com/nikever/osmosis/runs/6505857265?check_suite_focus=true

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? yes (arm64 support!)
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? yes
  • How is the feature or change documented? not applicable

@niccoloraspa niccoloraspa requested a review from a team May 19, 2022 11:40
@niccoloraspa
Copy link
Member Author

Should I target v9.x instead?

@niccoloraspa
Copy link
Member Author

niccoloraspa commented May 19, 2022

The same logic here could be used to create a "releaser" docker image that creates the binaries for both amd64 and arm64. Let me know if that could interest you!

@codecov-commenter
Copy link

codecov-commenter commented May 19, 2022

Codecov Report

Merging #1541 (c9553c7) into main (583cfec) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1541   +/-   ##
=======================================
  Coverage   19.46%   19.46%           
=======================================
  Files         242      242           
  Lines       32255    32255           
=======================================
  Hits         6279     6279           
  Misses      24822    24822           
  Partials     1154     1154           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 583cfec...c9553c7. Read the comment docs.

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

<3

@ValarDragon
Copy link
Member

Targetting main is fine, we can backport the change!

Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

@nikever could you also update the following image please:
https://github.com/osmosis-labs/osmosis/blob/82e58f8f5f7e200e6659de6490526391811dd9ea/contrib/images/rbuilder/Dockerfile

It is needed for building the release binaries

@niccoloraspa
Copy link
Member Author

niccoloraspa commented May 19, 2022

@nikever could you also update the following image please: https://github.com/osmosis-labs/osmosis/blob/82e58f8f5f7e200e6659de6490526391811dd9ea/contrib/images/rbuilder/Dockerfile

It is needed for building the release binaries

Since now we support different architectures, and each one would have to be linked against its corresponding CosmWasm library, what do you think about using goreleaser for this part instead of the Makefile?

It's pretty cool and automation-friendly: https://goreleaser.com/quick-start/

If you want to give it a shot, I think it's best to track this releasing part in another issue. lmk!

@p0mvn
Copy link
Member

p0mvn commented May 19, 2022

@nikever could you also update the following image please: https://github.com/osmosis-labs/osmosis/blob/82e58f8f5f7e200e6659de6490526391811dd9ea/contrib/images/rbuilder/Dockerfile
It is needed for building the release binaries

Since now we support different architectures, and each one would have to be linked against its corresponding CosmWasm library, what do you think about using goreleaser for this part instead of the Makefile?

It's pretty cool and automation-friendly: https://goreleaser.com/quick-start/

If you want to give it a shot, I think it's best to track this releasing part in another issue. lmk!

Yeah I'm all for looking into that later. We still need to make the current release image consistent with the main one so that we can make releases for all supported platforms. We can either do it here or in a separate issue

@niccoloraspa
Copy link
Member Author

niccoloraspa commented May 20, 2022

Yeah I'm all for looking into that later. We still need to make the current release image consistent with the main one so that we can make releases for all supported platforms. We can either do it here or in a separate issue

I'm tracking the release part in another issue: #1550 because it's quite tricky and it deserves its own PR to discuss the solution.

I have already started doing some work in this branch: https://github.com/osmosis-labs/osmosis/compare/main...nikever:feat/add-go-releaser?expand=1

We can merge this one since the Docker part should be okay.

@mergify mergify bot merged commit ba0c337 into osmosis-labs:main May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants