-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
[v2] Add s3 integration tests using directory buckets #8794
Conversation
8872d28
to
c5bb69e
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.
Quick interface comment, but otherwise looks good.
awscli/testutils.py
Outdated
if not region: | ||
region = 'us-west-2' | ||
client = session.create_client('s3', region_name=region) | ||
if not az: | ||
az = 'usw2-az1' |
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.
We might consider making the region
and az
input a tuple since they're inherently bound. i.e. if I pass us-east-1
but forget to add the az
, I don't think we want those disjoint, do we?
Something like this:
if not region: | |
region = 'us-west-2' | |
client = session.create_client('s3', region_name=region) | |
if not az: | |
az = 'usw2-az1' | |
if not location: | |
location = ('us-west-2', 'usw2-az1') | |
region, az = location | |
client = session.create_client('s3', region_name=region) |
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.
Only a nit on the naming and docs, looks good otherwise.
""" | ||
if not location: | ||
location = ('us-west-2', 'usw2-az1') | ||
region, az = location |
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.
nit: name the parameter availability_zone
instead of az
to avoid using abbreviations.
@@ -181,6 +181,45 @@ def create_bucket(session, name=None, region=None): | |||
return bucket_name | |||
|
|||
|
|||
def create_dir_bucket(session, name=None, location=None): |
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.
It would be helpful to a developer to put a docstring for the type of location
, since its a tuple.
s3_utils.delete_bucket(bucket_name) | ||
|
||
|
||
@pytest.fixture(scope='package') |
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.
Document this the same as shared_copy_bucket
:
# Directory bucket for cross-S3 copies
Set up S3 directory bucket creation/teardown for integration tests with high-level
s3
commands. For the existing teardown logic to be compatible with directory buckets, we switch fromListObjects
toListObjectsV2
when clearing out test buckets.