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
Validate instance type before CloudFormation #1007
Conversation
CloudFormation will try to validate the Default ECSInstanceType parameter based on the AllowedValues field which is problematic in regions that do not support our default t2.micro instance type such as eu-north-1. Resolves #1006
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.
Looks good, thank you!! Can you add manual test output in this PR too? Thanks!
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.
Overall LGTM just nits
param, err := cfnParams.GetParameter(ParameterKeyInstanceType) | ||
if err == cloudformation.ParameterNotFoundError { | ||
logrus.Infof("Defaulting instance type to %s", cloudformation.DefaultECSInstanceType) | ||
|
||
cfnParams.Add(ParameterKeyInstanceType, cloudformation.DefaultECSInstanceType) | ||
|
||
return cloudformation.DefaultECSInstanceType, nil | ||
} else if err != nil { | ||
return "", err | ||
} | ||
|
||
return aws.StringValue(param.ParameterValue), err |
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.
param, err := cfnParams.GetParameter(ParameterKeyInstanceType) | |
if err == cloudformation.ParameterNotFoundError { | |
logrus.Infof("Defaulting instance type to %s", cloudformation.DefaultECSInstanceType) | |
cfnParams.Add(ParameterKeyInstanceType, cloudformation.DefaultECSInstanceType) | |
return cloudformation.DefaultECSInstanceType, nil | |
} else if err != nil { | |
return "", err | |
} | |
return aws.StringValue(param.ParameterValue), err | |
param, err := cfnParams.GetParameter(ParameterKeyInstanceType) | |
if err == nil { | |
return aws.StringValue(param.ParameterValue), err | |
} | |
if err == cloudformation.ParameterNotFoundError { | |
logrus.Infof("Defaulting instance type to %s", cloudformation.DefaultECSInstanceType) | |
cfnParams.Add(ParameterKeyInstanceType, cloudformation.DefaultECSInstanceType) | |
return cloudformation.DefaultECSInstanceType, nil | |
} | |
return "", err |
Maybe like 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.
It's more standard to left-align the happy path. We should check for non-nil errors first and handle those.
Sample output runsUnsupported instance type flag
Default instance type is unsupported in specified region
Instance type flag is supported in specified region
|
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.
thank you!
I still need to run the integration tests against these proposed changes. |
Integ tests are passing now. I had to add |
Would be keen to get this merged. |
CloudFormation will try to validate the Default ECSInstanceType parameter
based on the AllowedValues field which is problematic in regions that do
not support our default t2.micro instance type such as eu-north-1.
Resolves #1006
Enter
[N/A]
in the box, if an item is not applicable to your change.Testing
Documentation
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.