-
Notifications
You must be signed in to change notification settings - Fork 6
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
Fix regex for ebs optimized setting #791
Conversation
@davidsiaw can you help us review this PR, please? |
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.
@matafc can we have a simple test that exercises this change?
allow(it).to receive(:instance_type) { 'm6i.large' }
expect(it).to be_ebs_optimized_by_default
@@ -26,7 +26,7 @@ class AutoscalingBuilder < CloudFormation::Builder | |||
|
|||
def ebs_optimized_by_default? | |||
# https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/EBSOptimized.html | |||
!!(instance_type =~ /\A(a1|c4|c5.?|d2|f1|g3.?|h1|i3|m4|m5.?|m6()?|p2|p3(dn)?|r4|r5.?|t3|u-.*|x1.?|z1d)\..*\z/) | |||
!!(instance_type =~ /\A(a1|c4|c5.?|d2|f1|g3.?|h1|i3|m4|m5.?|m6.+||p2|p3(dn)?|r4|r5.?|t3|u-.*|x1.?|z1d)\..*\z/) |
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.
might be an excess |
character here
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.
Ugh, thanks again.
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.
Added some tests for this now.
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
Fix for #789
#789 has been deployed, but not applied to any districts yet. We will deploy this fix and then proceed with updating the districts.
It seems that all newer instance types support this, we just want to make sure this flag is enabled for m6 types in the Cloudformation when updating.
This should match any m6 instance type defined at https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ebs-optimized.html#ebs-optimization-support