Use list_objects_v2 to check isdir #265

fchorney · 2022-08-02T20:57:00Z

isdir for an s3 directory currently calls s3_list_buckets to check if a top level bucket is a directory. I ran into an issue where the role I was using didn't have permissions to call list_buckets.

This change essentially calls S3.list_objects_v2 with max-keys set to 0 as we don't actually need to retrieve any keys. If the bucket exists and you have permissions for it, it will return with no error, but if the bucket doesn't exist we return false or raise an exception if some other error occurred.

Not too sure if this is exactly how we want to go about this (re: the exceptions), so if you have any suggestions let me know. I don't think there should be any issues for using S3.list_objects_v2 instead of s3_list_buckets.

src/s3path.jl

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

rofinn · 2022-08-03T16:09:04Z

Since this is just changing the behaviour for root bucket paths I think it's fine. You should add tests for each of the conditions though:

Normal isdir (probably already exists)
No such bucket error
Any other error

mattBrzezinski · 2022-08-03T19:28:00Z

I think these changes are fine, but don't really solve the root problem here, which is to add the permissions needed to the role? We're now requiring users to have permissions to ListObjects on a bucket, instead of ListBuckets.

However, I do think those are the more correct permissions to require. I would also tag this as a 0.10.0 release instead of 0.9.X as it could be breaking for some users.

fchorney · 2022-08-03T19:35:12Z

I think these changes are fine, but don't really solve the root problem here, which is to add the permissions needed to the role? We're now requiring users to have permissions to ListObjects on a bucket, instead of ListBuckets.

However, I do think those are the more correct permissions to require. I would also tag this as a 0.10.0 release instead of 0.9.X as it could be breaking for some users.

You already need ListObjects to check exists(fp) which happens on a non root bucket, so in this case you would theoretically already have ListObjects if you're calling readdir or isdir

mattBrzezinski · 2022-08-03T19:37:44Z

You already need ListObjects to check exists(fp) which happens on a non root bucket, so in this case you would theoretically already have ListObjects if you're calling readdir or isdir

Oh rad, I missed that! Just needs test cases and it should be good to go!

Side thought, made we should include in our documentation the permissions users would need to run these functions?

src/s3path.jl

test/runtests.jl

mattBrzezinski · 2022-08-04T18:07:20Z

test/s3path.jl

+        @test isdir(path) == true
+
+        # No Such Bucket
+        path = S3Path("s3://some_non_existent_bucket_7051649213"; config=config)


We should make this an actual AWSException w/ the NoSuchBucket error so we're not dependent on someone not making this an actual bucket.

fchorney · 2022-08-04T18:39:15Z

bors try

bors · 2022-08-04T18:50:12Z

try

Build succeeded:

test/s3path.jl

github-actions · 2022-08-04T19:26:42Z

test/s3path.jl

+                code,
+                "",
+                nothing,
+                AWS.HTTP.Exceptions.StatusError(404, "", "", ""),
+                nothing,


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

code,

"",

nothing,

AWS.HTTP.Exceptions.StatusError(404, "", "", ""),

nothing,

code, "", nothing, AWS.HTTP.Exceptions.StatusError(404, "", "", ""), nothing

github-actions · 2022-08-04T19:26:42Z

test/s3path.jl

+                patch = @patch AWSS3.S3.list_objects_v2(
+                    args...; kwargs...
+                ) = throw(test_exception)


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

patch = @patch AWSS3.S3.list_objects_v2(

args...; kwargs...

) = throw(test_exception)

patch = @patch function AWSS3.S3.list_objects_v2(args...; kwargs...)

throw(test_exception)

end

github-actions · 2022-08-04T19:26:42Z

test/s3path.jl

+                patch = @patch AWSS3.S3.list_objects_v2(
+                    args...; kwargs...
+                ) = throw(test_exception)


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

patch = @patch AWSS3.S3.list_objects_v2(

args...; kwargs...

) = throw(test_exception)

patch = @patch function AWSS3.S3.list_objects_v2(args...; kwargs...)

throw(test_exception)

end

rofinn

Seems fine to me. I do think we should bump the minor release though.

rofinn · 2022-08-03T21:54:30Z

Project.toml

@@ -1,6 +1,6 @@
 name = "AWSS3"
 uuid = "1c724243-ef5b-51ab-93f4-b0a88ac62a95"
-version = "0.9.8"
+version = "0.9.9"


Technically we're changing behaviour that folks could depend on?

Suggested change

version = "0.9.9"

version = "0.10.0"

I'm somewhat unsure if that's the case, what behaviour could someone depend on? Theoretically you already need list_objects permissions to do an isdir on a non root bucket. I guess this could potentially be an issue for someone that is only doing an isdir on a root bucket somehow? I'm fine to bump the minor if you still think that should be the case

Bumped the version regardless

fchorney · 2022-08-04T19:53:45Z

bors r+

265: Use list_objects_v2 to check isdir r=fchorney a=fchorney isdir for an s3 directory currently calls `s3_list_buckets` to check if a top level bucket is a directory. I ran into an issue where the role I was using didn't have permissions to call `list_buckets`. This change essentially calls `S3.list_objects_v2` with `max-keys` set to `0` as we don't actually need to retrieve any keys. If the bucket exists and you have permissions for it, it will return with no error, but if the bucket doesn't exist we return false or raise an exception if some other error occurred. Not too sure if this is exactly how we want to go about this (re: the exceptions), so if you have any suggestions let me know. I don't think there should be any issues for using `S3.list_objects_v2` instead of `s3_list_buckets`. Co-authored-by: Fernando Chorney <[email protected]> Co-authored-by: Fernando Chorney <[email protected]>

bors · 2022-08-04T19:55:39Z

Build failed:

Julia 1 - ubuntu-latest - x64

fchorney · 2022-08-04T20:12:16Z

This is frustrating. AWSS3.jl depends on Minio.jl for tests, but Minio.jl depends on AWSS3.jl and its deps don't allow for anything above 0.9

ExpandingMan · 2022-08-04T21:31:54Z

I would have to not only push the change to master but actually tag the commit for this to resolve the issue here. Can I get some assurances that this is what will be tagged as 0.10 before I do this? I don't think this is the typical procedure for how this is done, but none are ideal so I will go along with it, I just don't want to have Minio.jl sitting around with 0.10 and then somebody tag a different 0.10 where it breaks.

ExpandingMan · 2022-08-04T21:38:59Z

Also, FYI, I am getting my own CI/CD failures on Minio.jl right now and I have no clue why. I have successful tests locally with multiple different binaries so I don't know what's going on.

I'm going to try updating the package to use minio_jll.

fchorney · 2022-08-05T02:53:34Z

Minio.jl sitting around with 0.10 and then somebody tag a different 0.10 where it breaks

This needs to get in ASAP for something I am working on, so I think it makes sense that this will definitely be the 0.10.0 release.

mattBrzezinski · 2022-08-05T13:54:02Z

Minio.jl sitting around with 0.10 and then somebody tag a different 0.10 where it breaks

This needs to get in ASAP for something I am working on, so I think it makes sense that this will definitely be the 0.10.0 release.

Tagging this as a 0.10.0 release means we're stuck in this awkward situation when we want to do a 0.11.0 release. I'm not sure what people think, but I'm fine with moving forward to a 1.0.0 release here. The package is stable enough, and we've agreed upon being fine with doing major version bumps more frequently with breaking changes.

This would punt the need for this awkward version bump except in the case of a 2.0.0 release, but I do not foresee anyone having the time to do a major re-write of this package any time soon.

Any thoughts? @fchorney @rofinn @ExpandingMan

fchorney · 2022-08-05T16:16:51Z

sure I'm fine with going to 1.0.0

rofinn · 2022-08-05T16:21:35Z

I'm not sure what people think, but I'm fine with moving forward to a 1.0.0 release here.

Yeah, I think the core behaviour hasn't changed much in the last couple years and I don't think we have a roadmap for major breaking changes. If we ever get around to doing the FilePathsBase redesign we've been discussing for a while that can just be a 2.0 release.

https://gitlab.com/ExpandingMan/FilePaths2.jl

awkward situation when we want to do a 0.11.0 release

I don't think the awkward dependency situation should guide our decision on tagging a 1.0 release though. We should probably plan to resolve the package interdependencies regardless of which release we do. We should either merge minio into a module in AWSS3 or limit AWSS3 to a test dependency for minio?

fchorney · 2022-08-05T16:52:45Z

So in that case, we'd need minio.jl to allow AWSS3.jl v1 and then the tests here can pass and be merged in.

ExpandingMan · 2022-08-05T17:36:42Z

Minio.jl

I have just tagged Minio.jl 0.2.0 which nominally includes support for AWSS3.jl. Unfortunately it seems that the minio devs are on a warpath against single-node use cases which drastically limits its usefulness for unit testing. Besides what was mentioned in that thread, I have also noticed that minio is now rather idiosyncratic about what directory it wants to mount, even when empty; when trying to rewrite the unit tests I notice that it freaks out if you give it absolute paths like /tmp/somepath. I don't think any of the AWSS3.jl tests use pre-existing directories, but there may be some subtle changes needed somewhere to get tests passing again.

Sadly at this point I would not be totally surprised if minio ultimately became totally useless for us here, but there are not many alternatives. On the other hand, they have hit 1.0 so it'll likely be a while before they make further breaking changes.

tagging of AWSS3.jl

I strongly recommend we wait on tagging a 1.0 of this. In my opinion this package still has major issues that are unresolved, in particular we still lack a file path abstraction which is appropriate for all use cases. Some of my previous comments on that can be found here. In my opinion we still very much need a new or completely overhauled file paths package at which point the many "gotchas" in the weirdly idiosyncratic AWSS3.jl code can be eliminated.

By the way, I have not been working on FilePaths2.jl (either there or as an overhaul of FilePathsBase.jl). I'm satisfied I have figured out what is basically the "correct" approach, however there are still many engineering subtleties to be resolved, in particular syncing (i.e. between remote file systems and local path object). If there is a broad consensus that we really do need to get this done I might consider picking it up again, I was rather hesitant to put in a ton of effort just to wind up splitting the ecosystem in 2 or 3 pieces.

fchorney · 2022-08-05T18:09:36Z

bors try

bors · 2022-08-05T18:16:13Z

try

Build succeeded:

fchorney · 2022-08-05T19:48:58Z

since I need this to go in, is everyone fine with just keeping this 0.10 and we can do a 1.0 release later?

fchorney · 2022-08-07T00:26:55Z

bors r+

bors · 2022-08-07T00:32:35Z

Build succeeded:

Use list_objects_v2 to check isdir

beb513a

github-actions bot reviewed Aug 2, 2022

View reviewed changes

src/s3path.jl Outdated Show resolved Hide resolved

Update src/s3path.jl

05677bd

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

rofinn self-requested a review August 3, 2022 16:03

github-actions bot reviewed Aug 4, 2022

View reviewed changes

src/s3path.jl Outdated Show resolved Hide resolved

Add isdir Tests

38340c1

fchorney force-pushed the fc/readdir branch from 6234973 to 38340c1 Compare August 4, 2022 18:01

mattBrzezinski reviewed Aug 4, 2022

View reviewed changes

fchorney added 2 commits August 4, 2022 13:14

Make NoSuchBucket test a patch

a51c8a8

Sort usings

21d281a

bors bot added a commit that referenced this pull request Aug 4, 2022

Try #265:

92bb4c6

mjram0s approved these changes Aug 4, 2022

View reviewed changes

mattBrzezinski reviewed Aug 4, 2022

View reviewed changes

test/s3path.jl Outdated Show resolved Hide resolved

github-actions bot reviewed Aug 4, 2022

View reviewed changes

Restructure isdir tests

facf9ec

fchorney force-pushed the fc/readdir branch from 4ab585f to facf9ec Compare August 4, 2022 19:27

rofinn approved these changes Aug 4, 2022

View reviewed changes

Bump minor version

7915e9d

Add Minio 0.2 support

c05deca

bors bot added a commit that referenced this pull request Aug 5, 2022

Try #265:

f5526bf

bors bot merged commit 89493b5 into master Aug 7, 2022

bors bot deleted the fc/readdir branch August 7, 2022 00:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use list_objects_v2 to check isdir #265

Use list_objects_v2 to check isdir #265

fchorney commented Aug 2, 2022

rofinn commented Aug 3, 2022

mattBrzezinski commented Aug 3, 2022

fchorney commented Aug 3, 2022

mattBrzezinski commented Aug 3, 2022

mattBrzezinski Aug 4, 2022

fchorney commented Aug 4, 2022

bors bot commented Aug 4, 2022

github-actions bot Aug 4, 2022

github-actions bot Aug 4, 2022

github-actions bot Aug 4, 2022

rofinn left a comment

rofinn Aug 3, 2022

fchorney Aug 4, 2022

fchorney Aug 4, 2022

fchorney commented Aug 4, 2022

bors bot commented Aug 4, 2022

fchorney commented Aug 4, 2022

ExpandingMan commented Aug 4, 2022

ExpandingMan commented Aug 4, 2022 •

edited

Loading

fchorney commented Aug 5, 2022

mattBrzezinski commented Aug 5, 2022

fchorney commented Aug 5, 2022

rofinn commented Aug 5, 2022

fchorney commented Aug 5, 2022

ExpandingMan commented Aug 5, 2022 •

edited

Loading

fchorney commented Aug 5, 2022

bors bot commented Aug 5, 2022

fchorney commented Aug 5, 2022 •

edited

Loading

fchorney commented Aug 7, 2022

bors bot commented Aug 7, 2022

Use list_objects_v2 to check isdir #265

Use list_objects_v2 to check isdir #265

Conversation

fchorney commented Aug 2, 2022

rofinn commented Aug 3, 2022

mattBrzezinski commented Aug 3, 2022

fchorney commented Aug 3, 2022

mattBrzezinski commented Aug 3, 2022

mattBrzezinski Aug 4, 2022

Choose a reason for hiding this comment

fchorney commented Aug 4, 2022

bors bot commented Aug 4, 2022

try

github-actions bot Aug 4, 2022

Choose a reason for hiding this comment

github-actions bot Aug 4, 2022

Choose a reason for hiding this comment

github-actions bot Aug 4, 2022

Choose a reason for hiding this comment

rofinn left a comment

Choose a reason for hiding this comment

rofinn Aug 3, 2022

Choose a reason for hiding this comment

fchorney Aug 4, 2022

Choose a reason for hiding this comment

fchorney Aug 4, 2022

Choose a reason for hiding this comment

fchorney commented Aug 4, 2022

bors bot commented Aug 4, 2022

fchorney commented Aug 4, 2022

ExpandingMan commented Aug 4, 2022

ExpandingMan commented Aug 4, 2022 • edited Loading

fchorney commented Aug 5, 2022

mattBrzezinski commented Aug 5, 2022

fchorney commented Aug 5, 2022

rofinn commented Aug 5, 2022

fchorney commented Aug 5, 2022

ExpandingMan commented Aug 5, 2022 • edited Loading

Minio.jl

tagging of AWSS3.jl

fchorney commented Aug 5, 2022

bors bot commented Aug 5, 2022

try

fchorney commented Aug 5, 2022 • edited Loading

fchorney commented Aug 7, 2022

bors bot commented Aug 7, 2022

ExpandingMan commented Aug 4, 2022 •

edited

Loading

ExpandingMan commented Aug 5, 2022 •

edited

Loading

fchorney commented Aug 5, 2022 •

edited

Loading