Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(efs): replicating file systems #29347
Changes from 17 commits
c426b12
6dcf485
b1361d8
f31fc43
2737c3f
9956a3b
9a101c7
75dc9da
c575735
8537be0
9cb4146
2b664b2
2525202
e73da37
41dcee4
f77fb2c
6be4342
0fd10b0
1d19f2f
6914efd
ee9b069
67e4a90
8cff88a
3bf0713
e666eeb
0db02bb
f9f7879
831b22f
ad40a5c
14d5256
9cdb6eb
7e24d32
9b6bba7
dba033b
aeeaae8
495d41a
1b2e48b
ef15ab1
a3f177f
1d9092c
1700075
9360944
ffb634f
786643b
37cea2b
6f1c6dd
163b45a
1ac2a5e
f202048
e41e3f3
869b1a0
85c33eb
46f81da
6aa6666
edf0c08
59e8cd4
7c04a23
c26a21f
b806c5e
7e3380f
ecfd8c0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Since there are multiple ways to configure the destination file system in
ReplicationConfiguration
, I'd suggest making this an enum-like class with static functions to enforce stronger typing and coupling between required properties. For example:This should also reduce the need for some of the validation/error-handling below which is ideal.
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.
Sounds good! I'll try to implement it.
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?