Skip to content
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): support for r7i instance type in aws-ec2 #27737

Merged
merged 15 commits into from
Nov 9, 2023

Conversation

kevntao
Copy link
Contributor

@kevntao kevntao commented Oct 27, 2023

Support of R7i EC2-Instance type in aws-ec2.

The R7i was recently introduced in October 2023.
The R7i instance type has been confirmed to be supported in AWS-Cloudformation.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@aws-cdk-automation aws-cdk-automation requested a review from a team October 27, 2023 21:59
@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Oct 27, 2023
@kevntao
Copy link
Contributor Author

kevntao commented Oct 27, 2023

Exemption Request

This PR is similar to this PR, which didn't need changes to the README or tests.

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Oct 27, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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 has failed. See the aws-cdk-automation comment below for failure reasons. 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.

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 27, 2023
@scanlonp scanlonp self-assigned this Oct 30, 2023
@scanlonp scanlonp added pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-test The PR linter will not require test changes pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. labels Oct 30, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review October 30, 2023 17:09

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

Copy link
Contributor

@scanlonp scanlonp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc string for r7i is the same as r7iz. There is also no "word" representation of the instance. Can we add one?

I suspect that MEMORY7_INTEL should have corresponded to r7i, but it is coupled with r7iz. We cannot change that now, but maybe we can come up with a good name for r7i. (Also we can put a comment that this is a 1-time deviation from our pattern.)

@@ -326,6 +326,11 @@ export enum InstanceClass {
*/
R7GD = 'r7gd',

/**
* Memory optimized instances based on Intel Xeon Scalable (Sapphire Rapids) processors, 7th generation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets change this to not be the same as r7iz. Or if the better change would be to that doc string, do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree, i had difficulty finding the nuanced difference between the r7i and r7iz, unfortunately you're correct, the MEMORY7_INTEL should have gone to the r7i instead of the r7iz, but I had thought the r7iz was the replacement for the r7i since it used the same processor generation as the c7i and m7i.

I'm totally open to suggestions, but happy to take a stab at it for community review.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that mistake is on me. Do what you think looks good and we can go from there!

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 1, 2023
@mergify mergify bot dismissed scanlonp’s stale review November 2, 2023 19:18

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 2, 2023
@kevntao kevntao requested a review from scanlonp November 6, 2023 20:44
Copy link
Contributor

@scanlonp scanlonp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the MEMORY7_INTEL_BASE. Hopefully we get back on track with R8!

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 25906bb
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Copy link
Contributor

mergify bot commented Nov 9, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 9a56bce into aws:main Nov 9, 2023
10 checks passed
@kevntao kevntao deleted the kevntao/ec2-r7i branch November 9, 2023 20:45
mikewrighton pushed a commit that referenced this pull request Nov 13, 2023
Support of R7i EC2-Instance type in aws-ec2.

The [R7i](https://aws.amazon.com/ec2/instance-types/r7i/) was recently introduced in October 2023.
The R7i instance type has been confirmed to be supported in [AWS-Cloudformation](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-instance.html#cfn-ec2-instance-instancetype).

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-test The PR linter will not require test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants