-
Notifications
You must be signed in to change notification settings - Fork 9
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: enable CI to build base image for arm64 and amd64 #96
Conversation
d1cf1b0
to
caa3ace
Compare
caa3ace
to
0f9308f
Compare
be8c143
to
5b4feb9
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.
Couple nits, but otherwise LGTM! Thanks for this
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 matrix the branch as well so that we only run the arm package building for both cpp and python on main instead of on every pull request? Because the arm package building lengthens the pipeline, and we just put effort into reducing its length recently
Co-authored-by: Antoine Paletta <98616558+apaletta3@users.noreply.github.com>
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.
Just 1 Q. Also you might need to increase the timeout as building the docker image is taking > 6 hours O_O
c8ee99e
to
2957a16
Compare
@guillaumeloftorbital How long does it take using the cache in the CI? Also were you able to confirm that it is able to use the cache from one build to another? 🤔 Looks great man, thanks for the awesome addition |
@Derollez the CI takes ~6h but with cache is faster this job only take 1min for example https://github.com/open-space-collective/open-space-toolkit/actions/runs/8038419090 |
my issue with disabling the build for arm64 is that it could be easy to break the build but we will only see on release |
@guillaumeloftorbital No, I don't mean only on release, I mean only on main and also on release. So if it breaks on main after we merge we'll see it right away and not have to wait for a release. We already do this in ostk-astro with validation tests (kind of like integration tests) that only run on main to keep the length of the CI down. So could you make this change please? |
2011d87
to
5501a1e
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
Description
Testing
https://github.com/open-space-collective/open-space-toolkit-core/actions/runs/8030904722