-
Notifications
You must be signed in to change notification settings - Fork 25
ROX-29116: (fix) Use ARM GH action workflow runners for ARM builds #2106
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
Conversation
5f37929
to
2ec7e40
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.
Hey @robbycochran - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider extracting the creation of
ansible/secrets.yml
into a reusable component to avoid duplication between local and remote build jobs.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #2106 +/- ##
==========================================
- Coverage 28.52% 28.51% -0.01%
==========================================
Files 94 94
Lines 5757 5759 +2
Branches 2547 2547
==========================================
Hits 1642 1642
- Misses 3393 3395 +2
Partials 722 722
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
a runtime computed value within a job matrix.
GHA and its tiny little corner cases...
I would recommend adding the run-multiarch-builds
label before merging to make sure power and Z build correctly.
- name: Check arches for local build | ||
if: ${{ ! contains(inputs.architectures, 'ppc64le') }} | ||
id: arch | ||
run: echo "local-exclude=[{\"arch\":\"ppc64le\"}]" >> "$GITHUB_OUTPUT" |
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.
Not something we need to do as part of this PR, but it might be easier to create the excludes
array in the init.yml
workflow, then pass it in to the workflows that need it. We could probably replace the architectures
input entirely, but that might require some jq
magic for us to translate the JSON to strings in the manifest script. I'll look into it after this is merged if you want to ignore this comment.
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.
Agree -- we could also do something similar to stackrox and create a single object with named matrix strategies: https://github.com/stackrox/stackrox/blob/master/.github/workflows/build.yaml#L28
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.
Ok, but can we use python instead of bash? Because that thing is ungodly with all the $(jq ...)
in there
9aa0938
to
014e5b9
Compare
This reverts commit 61cd323.
Co-authored-by: Mauro Ezequiel Moltrasio <[email protected]>
192fb64
to
baddaf5
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.
Changes LGTM! Just left a minor nitpick that you can ignore.
Looks like there was a CI failure creating a Z VM, I'd encourage having as green a CI as possible before merging, but the error seems unrelated to the changes.
Co-authored-by: Mauro Ezequiel Moltrasio <[email protected]>
adds stable, beta, and dev channels
After the changes introduced in #2106, we are building arm64 images by default and trying to test them. If a change is made to the integration tests, we are only building the amd64 version of the container and the arm64 tests fail when trying to pull the image for that architecture. This patch makes it so the test container is built for amd64 and arm64 by default.
After the changes introduced in #2106, we are building arm64 images by default and trying to test them. If a change is made to the integration tests, we are only building the amd64 version of the container and the arm64 tests fail when trying to pull the image for that architecture. This patch makes it so the test container is built for amd64 and arm64 by default.
Description
Undo the revert with a fix for error of using a runtime computed value within a job matrix.
Re-enable changes from #2084 which was reverted. Now with fixes to github actions on these commits
Original Description:
Utilize newly added arm64 runners for github actions.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
TODO(replace-me)
Use this space to explain how you tested your PR, or, if you didn't test it, why you did not do so. (Valid reasons include "CI is sufficient" or "No testable changes")
In addition to reviewing your code, reviewers must also review your testing instructions, and make sure they are sufficient.
For more details, ref the Confluence page about this section.