-
Notifications
You must be signed in to change notification settings - Fork 16
feat(python): Add Support for Legacy DBEC and Migration Examples #1938
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
Conversation
1b314f4
to
21d22dd
Compare
...ption/runtimes/python/src/aws_dbesdk_dynamodb/internaldafny/extern/InternalLegacyOverride.py
Outdated
Show resolved
Hide resolved
21d22dd
to
522b0f9
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.
Most comments are nits/cleanups.
I'm most interested in the other interfaces, so let's get started on that. There may be some fun stuff with how all DBESDK interfaces interact with the EncryptedClient that we should take a look at
...ption/runtimes/python/src/aws_dbesdk_dynamodb/internaldafny/extern/InternalLegacyOverride.py
Outdated
Show resolved
Hide resolved
...ption/runtimes/python/src/aws_dbesdk_dynamodb/internaldafny/extern/InternalLegacyOverride.py
Show resolved
Hide resolved
Examples/runtimes/python/Migration/src/ddbec_to_awsdbe/awsdbe/common.py
Outdated
Show resolved
Hide resolved
...ption/runtimes/python/src/aws_dbesdk_dynamodb/internaldafny/extern/InternalLegacyOverride.py
Outdated
Show resolved
Hide resolved
Examples/runtimes/python/Migration/src/ddbec_to_awsdbe/awsdbe/common.py
Outdated
Show resolved
Hide resolved
Examples/runtimes/python/Migration/src/ddbec_to_awsdbe/awsdbe/migration_step_1.py
Outdated
Show resolved
Hide resolved
Examples/runtimes/python/Migration/src/ddbec_to_awsdbe/awsdbe/migration_step_2.py
Outdated
Show resolved
Hide resolved
746f096
to
dbb6541
Compare
legacy_encrypted_table = LegacyEncryptedTable( | ||
table=ddb_table, | ||
materials_provider=cmp, | ||
) |
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.
Did you test what happens if someone provides a legacy EncryptedClient for use with the new EncryptedTable? (i.e. using clients that expect different item formats)
The InternalLegacyOverride code you wrote makes me think things are fine if this happens, but it would be great to double check.
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.
Chatted offline, this was tested manually and it works.
For posterity (i.e. legacy deprecation work), we will add integ tests to validate that this continues to work.
...ption/runtimes/python/src/aws_dbesdk_dynamodb/internaldafny/extern/InternalLegacyOverride.py
Show resolved
Hide resolved
Examples/runtimes/python/Migration/src/ddbec_to_awsdbe/awsdbe/paginator/migration_step_1.py
Show resolved
Hide resolved
Examples/runtimes/python/Migration/test/ddbec_to_awsdbe/awsdbe/client/test_migration_step_3.py
Outdated
Show resolved
Hide resolved
...ption/runtimes/python/src/aws_dbesdk_dynamodb/internaldafny/extern/InternalLegacyOverride.py
Outdated
Show resolved
Hide resolved
...ption/runtimes/python/src/aws_dbesdk_dynamodb/internaldafny/extern/InternalLegacyOverride.py
Outdated
Show resolved
Hide resolved
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 new tests are fantastic -- exhaustive and truly bar-raising for our team's forward compatibility coverage.
I had one question around the default params in a constructor, but looks good otherwise.
def create_encryption_config(legacy_encryptor=None, legacy_policy=None): | ||
"""Create a DynamoDbTableEncryptionConfig with optional legacy override.""" | ||
if legacy_encryptor is not None and legacy_policy is not None: |
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.
Does this actually get called with the params set to None
anywhere? I looked for a bit but couldn't find it, but this code is also dense (for good reason!) and I'm probably misreading.
If it's not used anymore we can probably remove it, or add cases where these params are actually None
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'll remove the default assignment.
c391d4a
to
e43f890
Compare
Issue #, if available:
Description of changes:
a.
EncryptedClient
b.
EncryptedPaginator
c.
EncryptedResource
d.
EncryptedTable
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.