-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add support for symbolic links #37
Conversation
- Add support for symbolic links in S3 - Check for symlinks during `read` and directory traversal - Add `S3Path.is_symlink`, `S3Path.symlink_to`, `_S3Accessor.symlink`, and `_S3Accessor.is_symlink` - Refactor `ObjectSummary` instantiation to a separate method to allow for a new `follow_symlinks` argument to be passed, which, if True, results in the final non-symlink `ObjectSummary` instance being returned - Fixes liormizr#35 Signed-off-by: Dan Ryan <dan.ryan@canonical.com>
Signed-off-by: Dan Ryan <dan.ryan@canonical.com>
Signed-off-by: Dan Ryan <dan.ryan@canonical.com>
Signed-off-by: Dan Ryan <dan.ryan@canonical.com>
Signed-off-by: Dan Ryan <dan.ryan@canonical.com>
Signed-off-by: Dan Ryan <dan.ryan@canonical.com>
1d0bad9
to
ea17a63
Compare
Signed-off-by: Dan Ryan <dan.ryan@canonical.com>
ea17a63
to
f66ad6c
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.
Hi @techalchemy looks like an interesting feature and a great start.
I have few questions before will start a proper code review.
first think - you didn't modify the lstat method this method looks like a great please to put the indication that this path is a symbolic link...
Also are you handling that if the path is symbolic link stat method will return the "target" info?
Also do we want to keep the old behavior (raise no support exception) if the bucket is not set as a website bucket type?
What do you think?
@@ -519,3 +520,25 @@ def test_unlink(s3_mock): | |||
S3Path("/test-bucket/fake_folder").unlink() | |||
with pytest.raises(IsADirectoryError): | |||
S3Path("/fake-bucket/").unlink() | |||
|
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.
How are you testing the change that you did in the _S3Accessor?
I think that we need to add a test that check glob/rglob/iterdir with symlink and not "normal" keys right?
read
and directory traversalS3Path.is_symlink
,S3Path.symlink_to
,_S3Accessor.symlink
,and
_S3Accessor.is_symlink
ObjectSummary
instantiation to a separate method to allowfor a new
follow_symlinks
argument to be passed, which, if True,results in the final non-symlink
ObjectSummary
instance beingreturned
Signed-off-by: Dan Ryan dan.ryan@canonical.com