-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(ec2): instance type properties #29507
base: main
Are you sure you want to change the base?
Conversation
Looking for feedback here 👋 This currently just a proof of concept to discuss the design. Once we settle on it, I'll add the missing properties (CPU, Memory, etc.), as well as generate the complete JSON data and static instance type values. |
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.
The pull request linter fails with the following errors:
❌ Features must contain a change to an integration test file and the resulting snapshot.
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed, add Clarification Request
to a comment.
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
228f212
to
071d8da
Compare
3caf4f8
to
cf8588a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #29507 +/- ##
=======================================
Coverage 66.96% 66.96%
=======================================
Files 329 329
Lines 18667 18667
Branches 3260 3260
=======================================
Hits 12501 12501
Misses 5839 5839
Partials 327 327
Flags with carried forward coverage won't be shown. Click here to find out more.
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
I don't quite understand the change and how the shell script was used. The script seems to be a manual invocation so whenever there's an update, does it require manually invoking this script? Given the size of the changes in this PR, it makes me a bit uncomfortable to review or approve a PR of this size without any tests. Would you be able to introduce any integ tests? Note that I manually removed the |
Adding a |
Issue # (if applicable)
Closes #12022.
Reason for this change
Provide CDK users with both a list of valid class/type combinations, as well as the properties associated to those types.
Additionally, these properties could be used internally to give user warnings/errors if they are associated to limitations, such as Nitro support, cluster networking, etc.
Description of changes
InstanceType.INSTANCE_CLASS_INSTANCE_SIZE
, both with and without its class aliasinstance-properties.json
file containing the complete output of theec2:DescribeInstanceTypes
API commandaws ec2 describe-instance-types | jq '.InstanceTypes | { InstanceTypes: map({key: .InstanceType, value: del(.InstanceType)}) | sort_by(.key) | from_entries } | .InstanceTypes' > packages/aws-cdk-lib/aws-ec2/data/instance-properties.json
instanceType.instanceProperties
, allowing CDK users to access themDescription of how you validated changes
I'm going to add tests to make sure the data is exposed correctly, but given this data is only meant to be informative, it shouldn't have an impact on deployments
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license