Skip to content

add test for ListObjectsV2 + fix config flag#7987

Merged
bentsku merged 4 commits intomasterfrom
fix-list-objects-v2
Mar 28, 2023
Merged

add test for ListObjectsV2 + fix config flag#7987
bentsku merged 4 commits intomasterfrom
fix-list-objects-v2

Conversation

@bentsku
Copy link
Contributor

@bentsku bentsku commented Mar 28, 2023 •
edited
Loading

It seemed we had an issue with ListObjectsV2 and Glue. Once again, the specs were not right about the root tag of the XML response of S3. However, the responses for ListObjects and ListObjectsV2 are using the same root tag ListBucketResult so we must merge the two wrongly named "containers" in the specs into one for both operation.

Also, I've realized that we merged this from our old S3 migration branch:

# Forces to use S3 ASF provider, while not loading -ext (does not have asf provider)
# TODO: will need to discuss the best way to do this with -ext, this is temporary for testing
if not os.environ.get("PROVIDER_OVERRIDE_S3"):
    os.environ["PROVIDER_OVERRIDE_S3"] = "asf"

So, removing it.
Also, thanks @silv-io for the hours long debugging session we've had to find this issue 😄

@coveralls
Copy link

coveralls commented Mar 28, 2023 •
edited
Loading

Coverage Status

Coverage: 81.87% (+0.005%) from 81.864% when pulling 810a28d on fix-list-objects-v2 into 296a257 on master.

@bentsku bentsku requested a review from alexrashed March 28, 2023 14:39
bentsku
bentsku commented Mar 28, 2023
View reviewed changes
tests/integration/s3/test_s3.py
)
snapshot.match(f"list-object-version-{param['Id']}", response)

def test_list_objects_v2_with_prefix(self, s3_client, s3_bucket, snapshot):
Copy link
Contributor Author

@bentsku bentsku Mar 28, 2023

Choose a reason for hiding this comment

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

oops, forgot to add the AWS validated marker, will do in another PR related to S3, don't want to relaunch the CI

Copy link
Member

@alexrashed alexrashed Mar 28, 2023

Choose a reason for hiding this comment

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

Please just add the marker right before the merge.

@bentsku bentsku marked this pull request as ready for review March 28, 2023 14:49
alexrashed
alexrashed approved these changes Mar 28, 2023
View reviewed changes
Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Wow, thanks for digging into this rabbit hole to find this issue! While the fix is a bit hacky, there's nothing else we can do right now with the (wrong) specs we get from S3 in botocore.

@bentsku bentsku self-assigned this Mar 28, 2023
@bentsku bentsku added the aws:s3 Amazon Simple Storage Service label Mar 28, 2023
@github-actions
Copy link

github-actions bot commented Mar 28, 2023

LocalStack integration with Pro

1 866 tests  +1   1 675 ✔️ +2   1h 24m 1s ⏱️ - 1m 40s
       1 suites ±0      191 💤  - 1 
       1 files   ±0          0 ❌ ±0 

Results for commit 810a28d. ± Comparison against base commit 296a257.

@bentsku bentsku merged commit 6f1be22 into master Mar 28, 2023
@bentsku bentsku deleted the fix-list-objects-v2 branch March 28, 2023 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexrashed alexrashed alexrashed approved these changes

@macnev2013 macnev2013 Awaiting requested review from macnev2013

@thrau thrau Awaiting requested review from thrau

Assignees

@bentsku bentsku

Labels

aws:s3 Amazon Simple Storage Service

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bentsku @coveralls @alexrashed