Add options to mkdir
#610
Add options to mkdir
#610
Conversation
gcsfs/tests/test_core.py
Outdated
| def test_mkdir_options(gcs): | ||
| gcs = GCSFileSystem(endpoint_url=gcs._endpoint) | ||
| if not gcs.on_google: | ||
| pytest.skip("emulator doesn't support IAM policies.") |
There was a problem hiding this comment.
Instead, we should mock to make sure that at least the right IAM config dict is being sent
gcsfs/tests/test_core.py
Outdated
|
|
||
|
|
||
| def test_mkdir_options(gcs): | ||
| gcs = GCSFileSystem(endpoint_url=gcs._endpoint) |
There was a problem hiding this comment.
Why make a new instance here?
gcsfs/tests/test_core.py
Outdated
|
|
||
| gcs.mkdir(TEST_CUSTOM_BUCKET, uniform_access=True, public_access_prevention=True) | ||
| info = gcs.info(TEST_CUSTOM_BUCKET) | ||
| gcs.rm(TEST_CUSTOM_BUCKET, recursive=True) |
There was a problem hiding this comment.
This should be a fixture to make sure the bucket is cleaned up even on failure
gcsfs/core.py
Outdated
| @@ -865,6 +867,12 @@ async def _mkdir( | |||
| enable_versioning: bool | |||
| If True, creates the bucket in question with object versioning | |||
| enabled. | |||
| uniform_access: bool | |||
| If True, creates the bucket in question with uniform access | |||
There was a problem hiding this comment.
I don't know if most people will know what "uniform access" means
gcsfs/core.py
Outdated
| @@ -837,6 +837,8 @@ async def _mkdir( | |||
| location=None, | |||
| create_parents=True, | |||
| enable_versioning=False, | |||
| uniform_access=False, | |||
There was a problem hiding this comment.
I wonder how many kwargs we should pass before we start to make some Options dataclass
There was a problem hiding this comment.
Fair. Would you be in favor of replicating the entire Buckets:Insert api? Or at least the gcloud storage buckets create options? I wanted to add these in particular because they are the default (and recommended) options for buckets created through the console UI.
Looks like some others we are missing are class/autoclass, retention policy and requester pays. Plus some more niche options.
There was a problem hiding this comment.
How about we allow "iamConfiguration" as a kwarg, so the caller can put whatever they like in it? We could also other arguments that the API has - then it's just a JSON blob we pass along.
There was a problem hiding this comment.
I think this is a cleaner approach with the kwargs. location and enable_versioning also get passed to the json body but I didn't want to break these.
I got rid of the previous test but lmk if you have a particular way you would like these tested. I found the emulator and on_google split a little confusing to work with, especially for bucket creation.
Also added some more helpful info and links in the docstring.
Noticed there was no way to specify a few common options on bucket creation via the IAM API.
I can modify the test if there is a better way, or remove entirely.