Skip to content
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

s3 fake directory fix #206

Merged

Conversation

GeorgeSabu
Copy link
Contributor

@GeorgeSabu GeorgeSabu commented Feb 6, 2022

  • Have fixed the fake s3 directory bug
  • Have fixed the test cases
  • Added some content to the file in the assets folder in test so that file size is not zero
  • Also fixed some warning that are coming while running test cases

@GeorgeSabu
Copy link
Contributor Author

@pjbull I have updated cloudpathlib/client.py and cloudpathlib/cloudpath.py as they caused warning while running test cases.

@GeorgeSabu
Copy link
Contributor Author

@pjbull There is a attr.exceptions.FrozenInstanceError that is coming while running test because of which tests are failing which I am not able to reproduce locally.

@jayqi
Copy link
Member

jayqi commented Feb 6, 2022

@GeorgeSabu the error is from #203

@GeorgeSabu
Copy link
Contributor Author

@jayqi oops didnt know about it. I did an empty commit to rerun the test to make sure its not some other thing. Thanks for the info.

@jayqi
Copy link
Member

jayqi commented Feb 6, 2022

Another thing to note here is that we've made some changes to _list_dir in #202 so there are going to be some merge conflicts.


Additionally, some comments about the changes:

@pjbull pjbull changed the base branch from master to s3-fake-dir-fix February 6, 2022 23:12
@pjbull pjbull changed the base branch from s3-fake-dir-fix to fix-s3-fake-dir February 6, 2022 23:14
@pjbull pjbull merged commit b83899a into drivendataorg:fix-s3-fake-dir Feb 6, 2022
@pjbull pjbull mentioned this pull request Feb 6, 2022
pjbull pushed a commit that referenced this pull request Feb 6, 2022
* [WIP]: taking care of the corner case folder created from S3

* Fix format issues

* [WIP]:updated test case for the s3 file directory test

* removed the linting issue

* Empty commit to rerun the test cases

Co-authored-by: Sabu George <sabugeorge@elucidata-sabs.local>
@GeorgeSabu
Copy link
Contributor Author

@jayqi As far as I understood the code the function iterdir which wraps the _list_dir. And the iterdir is been used as an iterator in download_to as well as copytree function. The reason it goes to recursion is that when S3 creates a fake directory what I understands it does is making a PUT request to "Key" test-folder/.

import boto3

client = boto3.client('s3')

response = client.put_object(
        Bucket='my-top-level-bucket',
        Body='',
        Key='test-folder/'
        )

Now 'test-folder/' behaves likes an object than a prefix. And as copytree calls copytree internally for its subdirectory causes for it to go into recursion. And same is the case with download_to function.

By checking the Size > 0 I am making sure that this recursion does not happen. I hope this clears.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants