-
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(efs): replicating file systems #29347
Conversation
dd939f8
to
a16bbbe
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.
Thanks 👍 I left some initial comments for some refactoring.
Please let me know if you have suggestions for further improvements.
* | ||
* @default - no replication | ||
*/ | ||
readonly replicationConfiguration?: ReplicationConfiguration; |
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.
Why aren't we allowing multiple destinations? (CFN declaration)
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.
CloudFormation supports an array of destinations, but the maximum number of elements is restricted to 1. Therefore, I did not implement support for multiple destinations.
Should I add support for it to accommodate future expansion?
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.
Tricky question.
I guess we can keep it like this for now and deprecate it in the future in favor of an array of configurations if necessary.
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.
I've updated to receive replicationConfiguration
as array.
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.
I apologize for misunderstanding your opinion.. Which implementation will you adopt?
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.
So what I would do here is actually make this an enum like class to future proof us against if/when the service team does allow for multiple regions. My suggestion would be to make ReplicationConfiguration
a class with static functions like the Schedule class. For now, we'll only have one function that takes in one replicationConfiguration
, but in the future we can add more functions for multiple.
Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
b545233
to
41dcee4
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.
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.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
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.
Well shit. I have a bunch of review comments in pending. They're from a hot minute ago so some may be outdated.
* | ||
* @default - create regional file system for the replication destination | ||
*/ | ||
readonly availabilityZone?: string; |
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.
If the availability zone is only a valid field if the region is provided, these two should fields should be a single object.
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.
Further down I'm also seeing a lot of checking for whether certain properties are compatible with one another. This tells me that the contract here is too loose. I get that you are working from a construct that uses a lot of optional props when they really shouldn't be but I'd like further changes to not follow that pattern.
Can we structure this in a way that enforces the dependent props in the contract and disallows the combinations you're checking for below?
* | ||
* @default - no replication | ||
*/ | ||
readonly replicationConfiguration?: ReplicationConfiguration; |
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.
So what I would do here is actually make this an enum like class to future proof us against if/when the service team does allow for multiple regions. My suggestion would be to make ReplicationConfiguration
a class with static functions like the Schedule class. For now, we'll only have one function that takes in one replicationConfiguration
, but in the future we can add more functions for multiple.
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.
Final tiny docstring comments and then I'm good to approve :)
* | ||
* @default - create a new file system for the replication |
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.
* | |
* @default - create a new file system for the replication |
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.
@gracelu0
I believe the @ default description is essential because the destinationFileSystem
is optional. Removing this description could cause the CI to fail.
What do you think?
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.
Ohh right, thanks for pointing that out! perhaps “None” or “No destinationFileSystem set” instead then?
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! I've updated it.
Co-authored-by: Grace Luo <54298030+gracelu0@users.noreply.github.com>
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 for contributing!
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). |
Issue # (if applicable)
Closes #21455.
Reason for this change
EFS supports replicating file systems but AWS CDK cannot configure it.
Description of changes
Add
replicationConfiguration
toFileSystemProps
Description of how you validated changes
I have added both unit and integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license