Remove Boto resource fixtures#8674
Conversation
The aws_client_factory fixture supersedes this
The equivalent functionality offered by the client is now used
viren-nadkarni
added
the
semver: patch
| @pytest.fixture | ||
| def s3_empty_bucket(aws_client): | ||
| """ | ||
| Returns a factory that given a bucket name, deletes all objects and deletes all object versions | ||
| """ | ||
| # Boto resource would make this a straightforward task, but our internal client does not support Boto resource | ||
| # FIXME: this won't work when bucket has more than 1000 objects | ||
| def factory(bucket_name: str): | ||
| response = aws_client.s3.list_objects_v2(Bucket=bucket_name) | ||
| objects = [{"Key": obj["Key"]} for obj in response["Contents"]] | ||
| aws_client.s3.delete_objects(Bucket=bucket_name, Delete={"Objects": objects}) | ||
|
|
||
| response = aws_client.s3.list_object_versions(Bucket=bucket_name) | ||
| object_versions = [ | ||
| {"Key": obj["Key"], "VersionId": obj["VersionId"]} | ||
| for obj in response.get("Versions", []) | ||
| ] | ||
| if object_versions: | ||
| aws_client.s3.delete_objects(Bucket=bucket_name, Delete={"Objects": object_versions}) | ||
|
|
||
| yield factory |
There was a problem hiding this comment.
This new fixture replaces the functionality offered by the Boto S3 Resource
There was a problem hiding this comment.
I'd probably prefer this to be a normal utility over a fixture but should be ok to leave as-is for now 👍
| "`connect_to_resource_external` is obsolete. Use `localstack.aws.connect`", | ||
| DeprecationWarning, | ||
| ) | ||
|
|
There was a problem hiding this comment.
With this PR, these utilities are no longer used in the codebase but I've added these warnings so that they're properly highlighted in the IDE.
Btw, Python 3.12 will have a decorator to do exactly this https://peps.python.org/pep-0702/
| item_ids = ("test", "test2", "test 3") | ||
| for item_id in item_ids: | ||
| dynamodb_table.put_item(Item={"id": item_id}) | ||
| aws_client.dynamodb.put_item(TableName=table_name, Item={"id": {"S": item_id}}) |
There was a problem hiding this comment.
The client needs the items to be typed. The Boto resource had an intrinsic type conversion so it wasn't necessary earlier
| objects = s3_resource.Bucket(s3_bucket).objects.all() | ||
| keys = [] | ||
| for obj in objects: | ||
| keys.append(obj.key) |
There was a problem hiding this comment.
The keys var wasn't used, hence removed
| def test_create_key(self, kms_client_for_region, kms_create_key, snapshot, aws_client): | ||
| region = "us-east-1" | ||
| kms_client = kms_client_for_region(region) | ||
| account_id = aws_client.sts.get_caller_identity()["Account"] |
There was a problem hiding this comment.
This call is replaced by the account_id fixture
viren-nadkarni
force-pushed
the
remove-boto-resource
branch
4 times, most recently
from
2d12c2e to
3e2d72a
Compare
viren-nadkarni
force-pushed
the
remove-boto-resource
branch
from
3e2d72a to
c08101d
Compare
viren-nadkarni
requested review from
baermat,
bentsku,
dfangl,
dominikschubert,
joe4dev,
macnev2013,
pinzon,
simonrw,
steffyP and
thrau
as code owners
viren-nadkarni
requested review from
calvernaz
and removed request for
baermat,
bentsku,
dfangl,
joe4dev,
macnev2013,
pinzon,
simonrw,
steffyP and
thrau
dominikschubert
left a comment
dominikschubert
left a comment
There was a problem hiding this comment.
Small nits, otherwise LGTM
Thanks so much for tackling this. 😅
The resource abstraction definitely didn't bring enough value to warrant maintaining its support besides the normal clients.
Love that we now finally have a unified abstraction and are quite close to remove the rest of the aws_stack clients soon 🚀
| @pytest.fixture | ||
| def s3_empty_bucket(aws_client): | ||
| """ | ||
| Returns a factory that given a bucket name, deletes all objects and deletes all object versions | ||
| """ | ||
| # Boto resource would make this a straightforward task, but our internal client does not support Boto resource | ||
| # FIXME: this won't work when bucket has more than 1000 objects | ||
| def factory(bucket_name: str): | ||
| response = aws_client.s3.list_objects_v2(Bucket=bucket_name) | ||
| objects = [{"Key": obj["Key"]} for obj in response["Contents"]] | ||
| aws_client.s3.delete_objects(Bucket=bucket_name, Delete={"Objects": objects}) | ||
|
|
||
| response = aws_client.s3.list_object_versions(Bucket=bucket_name) | ||
| object_versions = [ | ||
| {"Key": obj["Key"], "VersionId": obj["VersionId"]} | ||
| for obj in response.get("Versions", []) | ||
| ] | ||
| if object_versions: | ||
| aws_client.s3.delete_objects(Bucket=bucket_name, Delete={"Objects": object_versions}) | ||
|
|
||
| yield factory |
There was a problem hiding this comment.
I'd probably prefer this to be a normal utility over a fixture but should be ok to leave as-is for now 👍
| "x-amzn-trace-id": "Root=1-640e7ae9-63faf1aa71313dd3638b68fd" | ||
| "x-amzn-trace-id": "Root=1-64afea44-3087b2f50c440c1958c4d62e" |
There was a problem hiding this comment.
This is actually an invalid snapshot (before and now) since it won't validate against AWS. The unique value here would need to be replaced via a transformer to make it valid.
Since this hasn't been introduced in this PR, feel free to leave as-is though.
|
|
||
| # clean up | ||
| table.delete() | ||
| aws_client.dynamodb.delete_table(TableName=table_name) |
There was a problem hiding this comment.
nit: might be good while going through here anyway to do this in a safe manner (e.g. using the cleanups fixture instead)
There was a problem hiding this comment.
I have a TODO note to convert the constants on top of this file into fixture which will take care of this :) I'll address these in follow up PRs
3 participants
This PR removes all use of Boto resources and replaces it with equivalent functionality offered by our new internal client (which uses the Boto client underneath).
There are no plans to support Boto resources as part of our new internal AWS client library. Furthermore, the Boto team does not actively develop Boto resources.