-
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): support throughput on LaunchTemplate EBS volumes #30716
Conversation
The build failure:
This is nowhere near the first spurious build failure I've seen in just my third PR to this project, and the problems are not limited to this main CI job: https://github.com/aws/aws-cdk/actions/runs/9737293345/job/26869186116?pr=30716 has also failed for reasons unclear. It has proven to be challenging to contribute to aws-cdk. Contributors need to pass through an underspecified battery of PR label transitions in order that their PR be brought to a human being's attention so that it may be reviewed and merged. These transitions are automated and gated by CI success. But here is a PR where two of the gating CI jobs have spuriously failed, with no way for me to initiate a retry except by, I guess, rewording my commit message so that I get a different commit hash. And if contributors don't teach themselves to do this song and dance then their PR will get closed by automation after a period of inactivity, as happened to this changeset in #30317. Suggestions to maintainers:
In aggregate, you have crafted a fully automated, drawn out process for contributors to be (incorrectly, I think) ignored. Ignoring contributors is your prerogative as maintainers, but you could be achieving such outcomes with much less effort than this. |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
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.
Thanks 👍 Left some comments for adjustments
dc030fd
to
5899b6b
Compare
#30716 (comment) the log file is completely empty? |
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.
Thanks 👍
7773043
to
8cd144f
Compare
49b2b42
to
05531b1
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.
Maybe I missed it, but I only saw the validation and definition of the property throughput
, but I don't see where you assign it to CfnVolume
?
This is not using |
Ah I see, I missed that line of code. Thanks for the clarification. |
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.
LGTM, thank you!
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). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Fixes #24341.
Reason for this change
You currently can't specify
throughput
on LaunchTemplate EBS volumes.Description of changes
This support was simply not included in ec2 when it was added to autoscaling in #22441. I have copied that PR's implementation implementation to ec2 and similarly adapted its tests.
Description of how you validated changes
Unit and integration tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license