Refactor s3_object module to reduce complexity #1193
Conversation
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
ansibullbot
added
WIP
GomathiselviS
force-pushed
the
s3_obj_cleanup
branch
from
db03ba4
to
23a3ef6
Compare
ansibullbot
added
integration
plugins/modules/s3_object.py
Outdated
| else: | ||
| local_last_modified = os.path.getmtime(local_file) |
There was a problem hiding this comment.
| else: | |
| local_last_modified = os.path.getmtime(local_file) | |
| local_last_modified = os.path.getmtime(local_file) |
plugins/modules/s3_object.py
Outdated
| def s3_object_do_get(module, connection, s3_vars): | ||
|
|
||
| keyrtn = key_check(module, connection, s3_vars.bucket, s3_vars.object, version=s3_vars.version, validate=s3_vars.validate) | ||
| if keyrtn is False: |
There was a problem hiding this comment.
| if keyrtn is False: | |
| if not keyrtn: |
plugins/modules/s3_object.py
Outdated
| else: | ||
| module.fail_json(msg="Key %s does not exist." % s3_vars.object) |
There was a problem hiding this comment.
| else: | |
| module.fail_json(msg="Key %s does not exist." % s3_vars.object) | |
| module.fail_json(msg="Key %s does not exist." % s3_vars.object) |
| else: | ||
| module.fail_json(msg="Bucket parameter is required.") |
There was a problem hiding this comment.
| else: | |
| module.fail_json(msg="Bucket parameter is required.") | |
| module.fail_json(msg="Bucket parameter is required.") |
plugins/modules/s3_object.py
Outdated
| if s3_vars.bucket: | ||
| if delete_key(module, connection, s3_vars.bucket, s3_vars.object): | ||
| module.exit_json(msg="Object deleted from bucket %s." % s3_vars.bucket, changed=True) |
There was a problem hiding this comment.
| if s3_vars.bucket: | |
| if delete_key(module, connection, s3_vars.bucket, s3_vars.object): | |
| module.exit_json(msg="Object deleted from bucket %s." % s3_vars.bucket, changed=True) | |
| if s3_vars.bucket and delete_key(module, connection, s3_vars.bucket, s3_vars.object): | |
| module.exit_json(msg="Object deleted from bucket %s." % s3_vars.bucket, changed=True) |
plugins/modules/s3_object.py
Outdated
| if s3_vars.bucket: | ||
| if delete_bucket(module, connection, s3_vars.bucket): | ||
| module.exit_json(msg="Bucket %s and all keys have been deleted." % s3_vars.bucket, changed=True) |
There was a problem hiding this comment.
| if s3_vars.bucket: | |
| if delete_bucket(module, connection, s3_vars.bucket): | |
| module.exit_json(msg="Bucket %s and all keys have been deleted." % s3_vars.bucket, changed=True) | |
| if s3_vars.bucket and delete_bucket(module, connection, s3_vars.bucket): | |
| module.exit_json(msg="Bucket %s and all keys have been deleted." % s3_vars.bucket, changed=True) |
plugins/modules/s3_object.py
Outdated
| else: | ||
| module.fail_json(msg="Bucket parameter is required.") |
There was a problem hiding this comment.
| else: | |
| module.fail_json(msg="Bucket parameter is required.") | |
| module.fail_json(msg="Bucket parameter is required.") |
plugins/modules/s3_object.py
Outdated
| else: | ||
| # only use valid bucket acls when creating the bucket | ||
| s3_vars.permission = s3_vars.bucket_acl | ||
| module.exit_json(msg="Bucket created successfully", changed=create_bucket(module, connection, s3_vars.bucket, s3_vars.location)) |
There was a problem hiding this comment.
| else: | |
| # only use valid bucket acls when creating the bucket | |
| s3_vars.permission = s3_vars.bucket_acl | |
| module.exit_json(msg="Bucket created successfully", changed=create_bucket(module, connection, s3_vars.bucket, s3_vars.location)) | |
| # only use valid bucket acls when creating the bucket | |
| s3_vars.permission = s3_vars.bucket_acl | |
| module.exit_json(msg="Bucket created successfully", changed=create_bucket(module, connection, s3_vars.bucket, s3_vars.location)) |
There was a problem hiding this comment.
Looking at the tox report there's still a fair bit of code which could do with being hacked apart. Generally liking the work so far though.
plugins/modules/s3_object.py
Outdated
| @@ -680,14 +701,15 @@ def upload_s3file(module, s3, bucket, obj, expiry, metadata, encrypt, headers, s | |||
|
|
|||
| if 'ContentType' not in extra: | |||
There was a problem hiding this comment.
This stanza is a good candidate for breaking out into its own function
| def upload_s3file( | ||
| module, s3, bucket, obj, expiry, metadata, encrypt, headers, | ||
| src=None, content=None, acl_disabled=False | ||
| ): |
There was a problem hiding this comment.
Lines 680 through 683 look to be repeated in a couple of places, they'd be a good candidate for pulling out into their own function (generating the common encryption params)
Same goes for lines 684 through 693 (generating the metadata params)
plugins/modules/s3_object.py
Outdated
| for acl in module.params.get('permission'): | ||
| s3.put_object_acl(ACL=acl, Bucket=bucket, Key=obj) |
There was a problem hiding this comment.
This little for loop is repeated in multiple places, it, and its try/except is_boto3_error_code('AccessControlListNotSupported'): would be good candidates to pull out into their own little function
plugins/modules/s3_object.py
Outdated
|
|
||
| url = put_download_url(module, s3, bucket, obj, expiry) |
There was a problem hiding this comment.
This call duplicates the try/except block above it. The try/except block above (lines 635 through 640) should probably be removed
GomathiselviS
changed the title
plugins/modules/s3_object.py
Outdated
| except botocore.exceptions.EndpointConnectionError as e: # pylint: disable=duplicate-except | ||
| module.fail_json_aws(e, msg="Invalid endpoint provided") | ||
| except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e: # pylint: disable=duplicate-except | ||
| module.fail_json_aws(e, msg="Failed while looking up bucket (during bucket_check) %s." % bucket) | ||
| module.fail_json_aws(e, | ||
| msg="Failed while looking up bucket (during bucket_check) %s." % bucket) |
There was a problem hiding this comment.
exists is never changed. You can just drop the variable and end with return True.
GomathiselviS
requested review from
goneri,
gravesm,
jillr,
tremble and
abikouo
plugins/modules/s3_object.py
Outdated
| return content_type | ||
|
|
||
|
|
||
| def get_extra_params(module, encrypt, metadata=None): |
There was a problem hiding this comment.
You pass the whole module object just to access module.params['encryption_mode'] and module.params['encryption_kms_key_id']. I think it would be more elegant to just pass these 2 keys through the function arguments. This way you've got a better control on your function inputs.
Signed-off-by: GomathiselviS <gomathiselvi@gmail.com> Separate functions for each action removed unwanted file try/except block refactor Address review comments Fixed indentation Fix pep8 failures Unit tests Added more unit tests Formatted Fixed whitespace error
GomathiselviS
force-pushed
the
s3_obj_cleanup
branch
from
9790e5f
to
5e5c993
Compare
GomathiselviS
dismissed
abikouo’s stale review
Since there are 2 approvals, the PR is ready to merge. And the change that was requested has been done.
| extra["ContentType"] = get_content_type( | ||
| src, present=extra.get("ContentType") | ||
| ) |
There was a problem hiding this comment.
This changes the effect of the previous code and replaces the contents of extra["ContentType"] with the return value of get_content_type, which if extra already has "ContentType" (ie. present is truthy) returns None.
I think we should remove the present argument from get_content_type and then only call it if it's not present:
if "ContentType" not in extra:
extra["ContentType"] = get_content_type(src)
Signed-off-by: GomathiselviS gomathiselvi@gmail.com
SUMMARY
ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION