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(efs): support for new elastic throughputmode #23200

Merged
merged 13 commits into from
Dec 6, 2022
Merged

Conversation

smsruthi
Copy link
Contributor

@smsruthi smsruthi commented Dec 1, 2022


All Submissions:

Adding new Construct Runtime Dependencies:

  • This PR adds new construct runtime dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

smsruthi and others added 2 commits December 1, 2022 16:33
Adding the new enum for ThroughputMode and LifeCyclePolicy
@gitpod-io
Copy link

gitpod-io bot commented Dec 1, 2022

@github-actions github-actions bot added the p2 label Dec 1, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team December 1, 2022 21:52
@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Dec 1, 2022
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.

@mrgrain mrgrain changed the title feat(aws-efs): Adding the new enums for throughputmode and lifecyclepolicy feat(efs): Adding the new enums for throughputmode and lifecyclepolicy Dec 2, 2022
@mrgrain
Copy link
Contributor

mrgrain commented Dec 2, 2022

Hey @smsruthi thanks for the PR!

I've exempted the integration test as I don't think we need one here.

However, would you mind updating the README: https://github.com/aws/aws-cdk/tree/5a20554754beda761623cbd9c71934ab89ec2ff2/packages/%40aws-cdk/aws-efs#create-an-amazon-efs-file-system

Particular the part about replacement seems out of date now. (see Restrictions on switching throughput modes and changing provisioned amount)

Additionally, with "Elastic Throughput mode is available only for file systems that are configured with the General Purpose performance mode." we need to add some validation for this constraint. Similar to the one we have for PROVISIONED.

Lastly, we also need unit tests covering the validation cases.

@mrgrain mrgrain added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Dec 2, 2022
@mrgrain mrgrain self-assigned this Dec 2, 2022
@aws-cdk-automation aws-cdk-automation dismissed their stale review December 5, 2022 20:39

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

@sbyooki
Copy link

sbyooki commented Dec 5, 2022

  • You still need to put comma at the end of
performanceMode: PerformanceMode.MAX_IO

If fails the build

  • A test failed with this:
 Expected pattern: /PerformanceMode cannot be MaxIO when using ThroughputMode ELASTIC/
    Received message: "Property performanceMode cannot be MAX_IO when throughputMode is ELASTIC"

^^ Other than making the two strings the same, I suggest you change the language more towards ELASTIC ThroughputMode is not supported for file systems with PerformanceMode of MaxIO

@mrgrain mrgrain changed the title feat(efs): Adding the new enums for throughputmode and lifecyclepolicy feat(efs): support for new elastic throughputmode Dec 6, 2022
mrgrain
mrgrain previously requested changes Dec 6, 2022
Copy link
Contributor

@mrgrain mrgrain left a comment

Choose a reason for hiding this comment

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

Looks good now, thank you. I'll take care of the text changes.

packages/@aws-cdk/aws-efs/test/efs-file-system.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-efs/lib/efs-file-system.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed mrgrain’s stale review December 6, 2022 13:00

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify
Copy link
Contributor

mergify bot commented Dec 6, 2022

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 d7ae159 into aws:main Dec 6, 2022
brennanho pushed a commit to brennanho/aws-cdk that referenced this pull request Dec 9, 2022
----

### All Submissions:

* [ yes] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
brennanho pushed a commit to brennanho/aws-cdk that referenced this pull request Jan 20, 2023
----

### All Submissions:

* [ yes] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
brennanho pushed a commit to brennanho/aws-cdk that referenced this pull request Feb 22, 2023
----

### All Submissions:

* [ yes] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*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-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants