-
Notifications
You must be signed in to change notification settings - Fork 1
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
Build both ARM64 and x86-64 AMIs #355
Build both ARM64 and x86-64 AMIs #355
Conversation
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 👍
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 ✔
be83a8e
to
9a35c4f
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.
Two AMIs for the price of one- that's some 🌶️ 💩 !
@mcdonnnj - Please re-review this PR, as I believe it addresses the concerns you mentioned in cisagov/debian-packer#63. |
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.
This looks pretty solid to me. Strong work 💪💪💪 I have two small items of feedback for your consideration.
89421b1
to
e0412e7
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.
Agreed, splitting the historical AMIs is a better solution. 👍
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.
Looking even better! I have one item of feedback (in two places) for your consideration, but otherwise this LGTM ✔ Thanks again for separating the historical AMI configuration.
Note
Please add a PR pre-merge item to adjust the required tests since the build
job now uses a matrix.
This is more flexible than filtering the architecture just based on the name, particularly since the name filter does not support full-fledged regexes.
This should make the search more efficient.
This will allow us to build both x86-64 and ARM64 versions of our AMI.
In order that the builds should complete as soon as possible, the different architectures are built in parallel.
We want to be able to offer both wherever possible.
Co-authored-by: Nick <[email protected]>
…sions Co-authored-by: Nick <[email protected]>
Also reference issue #369 to consider removing the moved blocks when they are no longer necessary.
Co-authored-by: David Redmin <[email protected]>
e6e6549
to
88528fe
Compare
🗣 Description
This pull request modifies the Terraform code to
💭 Motivation and context
🧪 Testing
All automated tests pass. I also built staging AMIs with these changes and verified that they functioned as expected.
✅ Pre-approval checklist
✅ Pre-merge checklist
✅ Post-merge checklist