-
Notifications
You must be signed in to change notification settings - Fork 81
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
Dynamically filter out invalid instance types #517
Conversation
} | ||
|
||
func (m *NodegroupManager) getValidInstanceTypesFromList(desiredInstanceTypes []ec2types.InstanceType) []string { | ||
_, err := m.clients.EC2().DescribeInstanceTypes(context.TODO(), &ec2.DescribeInstanceTypesInput{ |
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.
You should be able to check the error code here like:
if err != nil {
var apierr smithy.APIError
if errors.As(err, &apierr) {
// do something with apierr.ErrorCode()
}
}
instead of doing the parsing yourself
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 pushed a change to parse the error code in this format and pass just the error message to the invalid instance types parser rather than the whole error.
c4c1f06
to
f902cc2
Compare
f902cc2
to
d59e57e
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.
Just some nits 👍
Co-authored-by: Carter <[email protected]>
ec36c56
to
28ed9f8
Compare
Description of changes:
Leverage
EC2:DescribeInstanceTypes
API to dynamically scope down default instance types list to only those that exist in the test region.Relies on error format:
Specifically, the error handler relies on the presence of
InvalidInstanceType
in the error string, a printed list of the instance types being discoverable by indexing the last occurrence of a forward square bracket ([
), and multiple types being separated by a comma and a space (,
).Tested this by appending
ec2types.InstanceTypeTrn1n32xlarge
to the defaultInstanceTypes_x86_64 array and then creating a test deployment in us-west-1. Verified that the 'Eliminating instance type' log line was hit only for trn1n.32xlarge and that node groups create.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.