Skip to content

feat(ec2): support throughput on LaunchTemplate EBS volumes#30317

Closed
isker wants to merge 1 commit intoaws:mainfrom
isker:ebs-throughput
Closed

feat(ec2): support throughput on LaunchTemplate EBS volumes#30317
isker wants to merge 1 commit intoaws:mainfrom
isker:ebs-throughput

Conversation

@isker
Copy link
Copy Markdown
Contributor

@isker isker commented May 23, 2024 •
edited
Loading

Issue # (if applicable)

Fixes #24341.

Reason for this change

This support was simply not included in ec2 when it was added to autoscaling in #22441.

Description of changes

I have copied that PR's implementation implementation to ec2 and similarly adapted its tests.

Description of how you validated changes

Unit tests.

Checklist

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

@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels May 23, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team May 23, 2024 02:08
@isker isker force-pushed the ebs-throughput branch from 6dde01b to e1d73f1 Compare May 23, 2024 02:09
aws-cdk-automation
aws-cdk-automation requested changes May 23, 2024
View reviewed changes
Copy link
Copy Markdown
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.

@isker
Copy link
Copy Markdown
Contributor Author

isker commented May 23, 2024 •
edited
Loading

Exemption Request

  1. The EC2 README already has nothing to say about GP3 on LaunchTemplate BlockDevice volumes. It does talk about GP3 on ec2.Volumes already. I am not sure it's worth showing GP3 usage for LaunchTemplates as well.
  2. The PR that added general throughput support to EC2 feat(autoscaling): support for throughput on GP3 volumes #22441 declined to add integration tests to EC2 and was still merged. I guess I'm similarly declining 🌞.

Perhaps this PR is better characterized as a fix? I'm not sure.

@github-actions github-actions bot mentioned this pull request May 27, 2024
Closed
@isker
Copy link
Copy Markdown
Contributor Author

isker commented May 29, 2024

Hi @pahud, since you have context on the linked issue, could you give me your thoughts on the above exemption request? Thanks!

@github-actions github-actions bot mentioned this pull request Jun 1, 2024
Closed
@isker
Copy link
Copy Markdown
Contributor Author

isker commented Jun 6, 2024

Hi @pahud sorry to ping you once again but I would appreciate your advice on how to proceed. Thanks.

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

aws-cdk-automation commented Jun 14, 2024

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.

@isker
Copy link
Copy Markdown
Contributor Author

isker commented Jun 14, 2024

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.

@pahud any help you can give me on this?

@isker isker mentioned this pull request Jun 17, 2024
Merged
1 task
@GavinZZ
Copy link
Copy Markdown
Member

GavinZZ commented Jun 17, 2024 •
edited
Loading

Hi @isker, EC2 is also a stable module so I would highly recommend adding an simple integration test to use the throughput property to make sure it's deployable. It should be quite straightforward. For the linked similar PR you mentioned, it also added an integ test.

@isker
Copy link
Copy Markdown
Contributor Author

isker commented Jun 17, 2024

@GavinZZ I see, I think I got confused because that PR added its test to packages/@aws-cdk/aws-autoscaling, and I saw no integration tests in packages/@aws-cdk/aws-ec2, but that's because everything got moved in a2c633f. I see where I can add integration test coverage for aws-ec2 now. I will do that.

@isker
feat(ec2): support throughput on LaunchTemplate EBS volumes
672fd0d 
This support was simply not included in ec2 when it was added to
autoscaling in aws#22441. I have copied that PR's implementation
implementation to ec2 and similarly adapted its tests.

Fixes aws#24341.
@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

aws-cdk-automation commented Jun 18, 2024

AWS CodeBuild CI Report

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

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

@isker
Copy link
Copy Markdown
Contributor Author

isker commented Jun 18, 2024

Alright, there's the integ coverage. I'm still not sure that this should be added to any README, for the reasons I already mentioned.

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

aws-cdk-automation commented Jun 22, 2024

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Jun 22, 2024
@isker
Copy link
Copy Markdown
Contributor Author

isker commented Jun 22, 2024

@GavinZZ help 😢

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

aws-cdk-automation commented Jun 22, 2024

The pull request linter fails with the following errors:

❌ Features must contain a change to a README file.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

✅ A exemption request has been requested. Please wait for a maintainer's review.

@GavinZZ GavinZZ reopened this Jun 23, 2024
@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

aws-cdk-automation commented Jun 24, 2024

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

This was referenced Jul 1, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aws-cdk-automation aws-cdk-automation aws-cdk-automation requested changes

Assignees

No one assigned

Labels

beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

EbsDeviceProps missing support for setting throughput on GP3 volumes, yet VolumeProps has support for it

3 participants

@isker @aws-cdk-automation @GavinZZ