Various pylint fixups (ansible-collections#2444)

SUMMARY
fixes various linting warnings:

redefined-builtin
redefined-outer-name
no-else-continue
simplifiable-if-statement
unused-import

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
plugins/modules/ec2_ami.py
plugins/modules/ec2_vpc_vpn.py
plugins/modules/s3_bucket.py
plugins/modules/s3_object.py
tests/unit/
ADDITIONAL INFORMATION
Also applies the "maybe_sleep" fixture to the ACM tests which have retries attached to them.

Reviewed-by: Alina Buzachis

changelogs/fragments/2444-pylint.yml

@@ -0,0 +1,7 @@
+---
+minor_changes:
+  - ec2_ami - avoid redefining ``delete_snapshot`` inside ``DeregisterImage.do`` (https://github.com/ansible-collections/amazon.aws/pull/2444).
+  - s3_bucket - avoid redefining ``id`` inside ``handle_bucket_inventory`` and ``delete_bucket_inventory`` (https://github.com/ansible-collections/amazon.aws/pull/2444).
+  - s3_object - avoid redefining ``key_check`` inside ``_head_object`` (https://github.com/ansible-collections/amazon.aws/pull/2444).
+  - s3_object - simplify ``path_check`` logic (https://github.com/ansible-collections/amazon.aws/pull/2444).
+  - ec2_vpc_vpn - minor linting fixups (https://github.com/ansible-collections/amazon.aws/pull/2444).

plugins/modules/ec2_ami.py

@@ -646,7 +646,6 @@ def timeout(connection, image_id, wait_timeout):
     @classmethod
     def do(cls, module, connection, image_id):
         """Entry point to deregister an image"""
-        delete_snapshot = module.params.get("delete_snapshot")
         wait = module.params.get("wait")
         wait_timeout = module.params.get("wait_timeout")
         image = get_image_by_id(connection, image_id)
@@ -671,7 +670,7 @@ def do(cls, module, connection, image_id):
 
         exit_params = {"msg": "AMI deregister operation complete.", "changed": True}
 
-        if delete_snapshot:
+        if module.params.get("delete_snapshot"):
             exit_params["snapshots_deleted"] = list(purge_snapshots(connection))
 
         module.exit_json(**exit_params)

plugins/modules/ec2_vpc_vpn.py

@@ -565,9 +565,9 @@ def create_filter(module, provided_filters: Dict[str, Any]) -> List[Dict[str, An
                 flat_filter_dict[param] = [str(provided_filters[param])]
 
     # if customer_gateway, vpn_gateway, or vpn_connection was specified in the task but not the filter, add it
-    for param in param_to_filter:
-        if param_to_filter[param] not in flat_filter_dict and module.params.get(param):
-            flat_filter_dict[param_to_filter[param]] = [module.params.get(param)]
+    for param, param_value in param_to_filter.items():
+        if param_value not in flat_filter_dict and module.params.get(param):
+            flat_filter_dict[param_value] = [module.params.get(param)]
 
     # change the flat dict into something boto3 will understand
     formatted_filter = [{"Name": key, "Values": value} for key, value in flat_filter_dict.items()]
@@ -710,7 +710,7 @@ def check_for_routes_update(client, module: AnsibleAWSModule, vpn_connection_id:
     for attribute in current_attrs:
         if attribute in ("tags", "routes", "state"):
             continue
-        elif attribute == "options":
+        if attribute == "options":
             will_be = module.params.get("static_only")
             is_now = bool(current_attrs[attribute]["static_routes_only"])
             attribute = "static_only"

plugins/modules/s3_bucket.py

@@ -1209,9 +1209,9 @@ def handle_bucket_inventory(s3_client, module: AnsibleAWSModule, name: str) -> T
 
         results.append(declared_inventory_api)
 
-    for id in present_inventories.keys():
+    for inventory_id in present_inventories.keys():
         try:
-            delete_bucket_inventory(s3_client, name, id)
+            delete_bucket_inventory(s3_client, name, inventory_id)
         except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e:
             module.fail_json_aws(e, msg="Failed to delete bucket inventory")
         bucket_changed = True
@@ -1476,7 +1476,7 @@ def put_bucket_tagging(s3_client, bucket_name: str, tags: dict):
 
 
 @AWSRetry.exponential_backoff(max_delay=120, catch_extra_error_codes=["NoSuchBucket", "OperationAborted"])
-def delete_bucket_inventory(s3_client, bucket_name: str, id: str) -> None:
+def delete_bucket_inventory(s3_client, bucket_name: str, inventory_id: str) -> None:
     """
     Delete the inventory settings for an S3 bucket.
     Parameters:
@@ -1486,7 +1486,7 @@ def delete_bucket_inventory(s3_client, bucket_name: str, id: str) -> None:
     Returns:
         None
     """
-    s3_client.delete_bucket_inventory_configuration(Bucket=bucket_name, Id=id)
+    s3_client.delete_bucket_inventory_configuration(Bucket=bucket_name, Id=inventory_id)
 
 
 @AWSRetry.exponential_backoff(max_delay=120, catch_extra_error_codes=["NoSuchBucket", "OperationAborted"])

plugins/modules/s3_object.py

@@ -500,13 +500,13 @@ def etag_compare(module, s3, bucket, obj, version=None, local_file=None, content
 def _head_object(s3, bucket, obj, version=None):
     try:
         if version:
-            key_check = s3.head_object(aws_retry=True, Bucket=bucket, Key=obj, VersionId=version)
+            obj_head = s3.head_object(aws_retry=True, Bucket=bucket, Key=obj, VersionId=version)
         else:
-            key_check = s3.head_object(aws_retry=True, Bucket=bucket, Key=obj)
-        if not key_check:
+            obj_head = s3.head_object(aws_retry=True, Bucket=bucket, Key=obj)
+        if not obj_head:
             return {}
-        key_check.pop("ResponseMetadata")
-        return key_check
+        obj_head.pop("ResponseMetadata")
+        return obj_head
     except is_boto3_error_code("404"):
         return {}
 
@@ -656,10 +656,7 @@ def create_dirkey(module, s3, bucket, obj, encrypt, expiry):
 
 
 def path_check(path):
-    if os.path.exists(path):
-        return True
-    else:
-        return False
+    return bool(os.path.exists(path))
 
 
 def guess_content_type(src):

tests/integration/targets/s3_object/library/test_s3_upload_multipart.py

@@ -93,7 +93,7 @@ def main():
     )
 
     bucket = module.params.get("bucket")
-    object = module.params.get("object")
+    object_key = module.params.get("object")
     part_size = module.params.get("part_size")
     parts = module.params.get("parts")
 
@@ -109,13 +109,13 @@ def main():
         module.fail_json_aws(e, msg="Failed to connect to AWS")
 
     # create multipart upload
-    response = s3.create_multipart_upload(Bucket=bucket, Key=object)
+    response = s3.create_multipart_upload(Bucket=bucket, Key=object_key)
     upload_id = response.get("UploadId")
 
     # upload parts
     upload_params = {
         "Bucket": bucket,
-        "Key": object,
+        "Key": object_key,
         "UploadId": upload_id,
     }
 
@@ -124,7 +124,7 @@ def main():
     # complete the upload
     response = s3.complete_multipart_upload(
         Bucket=bucket,
-        Key=object,
+        Key=object_key,
         MultipartUpload={"Parts": multiparts},
         UploadId=upload_id,
     )

tests/unit/conftest.py

@@ -0,0 +1,9 @@
+# This file is part of Ansible
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+# pylint: disable=unused-import
+
+import pytest
+
+from .utils.amazon_placebo_fixtures import fixture_maybe_sleep
+from .utils.amazon_placebo_fixtures import fixture_placeboify

tests/unit/module_utils/botocore/test_aws_region.py

@@ -23,17 +23,17 @@ class FailException(Exception):
     pass
 
 
-@pytest.fixture
-def aws_module(monkeypatch):
+@pytest.fixture(name="aws_module")
+def fixture_aws_module(monkeypatch):
     aws_module = MagicMock()
     aws_module.fail_json.side_effect = FailException()
     aws_module.fail_json_aws.side_effect = FailException()
     monkeypatch.setattr(aws_module, "params", sentinel.MODULE_PARAMS)
     return aws_module
 
 
-@pytest.fixture
-def fake_boto3(monkeypatch):
+@pytest.fixture(name="fake_boto3")
+def fixture_fake_boto3(monkeypatch):
     # Note: this isn't a monkey-patched real-botocore, this is a complete fake.
     fake_session = MagicMock()
     fake_session.region_name = sentinel.BOTO3_REGION
@@ -47,8 +47,8 @@ def fake_boto3(monkeypatch):
     return fake_boto3
 
 
-@pytest.fixture
-def botocore_utils(monkeypatch):
+@pytest.fixture(name="botocore_utils")
+def fixture_botocore_utils(monkeypatch):
     return utils_botocore
 
 

tests/unit/module_utils/botocore/test_boto3_conn.py

@@ -21,16 +21,16 @@ class FailException(Exception):
     pass
 
 
-@pytest.fixture
-def aws_module(monkeypatch):
+@pytest.fixture(name="aws_module")
+def fixture_aws_module(monkeypatch):
     aws_module = MagicMock()
     aws_module.fail_json.side_effect = FailException()
     monkeypatch.setattr(aws_module, "_name", sentinel.MODULE_NAME)
     return aws_module
 
 
-@pytest.fixture
-def botocore_utils(monkeypatch):
+@pytest.fixture(name="botocore_utils")
+def fixture_botocore_utils(monkeypatch):
     return utils_botocore
 
 

tests/unit/module_utils/botocore/test_connection_info.py

@@ -31,17 +31,17 @@ class FailException(Exception):
     pass
 
 
-@pytest.fixture
-def aws_module(monkeypatch):
+@pytest.fixture(name="aws_module")
+def fixture_aws_module(monkeypatch):
     aws_module = MagicMock()
     aws_module.fail_json.side_effect = FailException()
     aws_module.fail_json_aws.side_effect = FailException()
     monkeypatch.setattr(aws_module, "params", sentinel.MODULE_PARAMS)
     return aws_module
 
 
-@pytest.fixture
-def fake_botocore(monkeypatch):
+@pytest.fixture(name="fake_botocore")
+def fixture_fake_botocore(monkeypatch):
     # Note: this isn't a monkey-patched real-botocore, this is a complete fake.
     fake_session = MagicMock()
     fake_session.get_config_variable.return_value = sentinel.BOTO3_REGION
@@ -58,8 +58,8 @@ def fake_botocore(monkeypatch):
     return fake_botocore
 
 
-@pytest.fixture
-def botocore_utils(monkeypatch):
+@pytest.fixture(name="botocore_utils")
+def fixture_botocore_utils(monkeypatch):
     region_method = MagicMock(name="_aws_region")
     monkeypatch.setattr(utils_botocore, "_aws_region", region_method)
     region_method.return_value = sentinel.RETURNED_REGION
@@ -204,7 +204,7 @@ def test_aws_connection_info_validation(monkeypatch, botocore_utils, options, ex
     assert region is sentinel.RETURNED_REGION
     assert endpoint_url is None
     assert boto_params == expected_params
-    boto_params["verify"] is expected_validate
+    assert boto_params["verify"] == expected_validate
 
 
 def test_aws_connection_info_profile(monkeypatch, botocore_utils):

tests/unit/module_utils/botocore/test_merge_botocore_config.py

@@ -20,8 +20,8 @@
 }
 
 
-@pytest.fixture
-def basic_config():
+@pytest.fixture(name="basic_config")
+def fixture_basic_config():
     config = botocore.config.Config(**MINIMAL_CONFIG)
     return config
 

tests/unit/module_utils/cloud/test_decorator_generation.py

@@ -18,8 +18,8 @@
     )
 
 
-@pytest.fixture
-def patch_cloud_retry(monkeypatch):
+@pytest.fixture(name="patch_cloud_retry")
+def fixture_patch_cloud_retry(monkeypatch):
     """
     replaces CloudRetry.base_decorator with a MagicMock so that we can exercise the generation of
     the various "public" decorators.  We can then check that base_decorator was called as expected.

tests/unit/module_utils/cloud/test_retry_func.py

@@ -27,8 +27,8 @@ def __init__(self):
         pass
 
 
-@pytest.fixture
-def retrier():
+@pytest.fixture(name="retrier")
+def fixture_retrier():
     def do_retry(
         func=None,
         sleep_generator=None,

tests/unit/module_utils/conftest.py

@@ -16,8 +16,8 @@
 from ansible.module_utils.six import string_types
 
 
-@pytest.fixture
-def stdin(mocker, request):
+@pytest.fixture(name="stdin")
+def fixture_stdin(mocker, request):
     old_args = ansible.module_utils.basic._ANSIBLE_ARGS
     ansible.module_utils.basic._ANSIBLE_ARGS = None
     old_argv = sys.argv
@@ -56,8 +56,8 @@ def stdin(mocker, request):
     sys.argv = old_argv
 
 
-@pytest.fixture
-def am(stdin, request):
+@pytest.fixture(name="am")
+def fixture_am(stdin, request):
     old_args = ansible.module_utils.basic._ANSIBLE_ARGS
     ansible.module_utils.basic._ANSIBLE_ARGS = None
     old_argv = sys.argv

tests/unit/module_utils/ec2/test_determine_iam_role.py

@@ -37,14 +37,14 @@ def __init__(self):
         pass
 
 
-@pytest.fixture
-def ec2_utils_fixture(monkeypatch):
+@pytest.fixture(name="ec2_utils_fixture")
+def fixture_ec2_utils_fixture(monkeypatch):
     monkeypatch.setattr(ec2_utils, "validate_aws_arn", lambda arn, service, resource_type: None)
     return ec2_utils
 
 
-@pytest.fixture
-def iam_client():
+@pytest.fixture(name="iam_client")
+def fixture_iam_client():
     client = MagicMock()
     return client
 

tests/unit/module_utils/elbv2/test_listener_rules.py

@@ -11,8 +11,8 @@
 from ansible_collections.amazon.aws.plugins.module_utils import elbv2
 
 
-@pytest.fixture
-def elb_listener_rules(mocker):
+@pytest.fixture(name="elb_listener_rules")
+def fixture_elb_listener_rules(mocker):
     module = MagicMock()
     connection = MagicMock()
     rules = MagicMock()

tests/unit/module_utils/exceptions/test_exceptions.py

@@ -10,8 +10,8 @@
 import ansible_collections.amazon.aws.plugins.module_utils.exceptions as aws_exceptions
 
 
-@pytest.fixture
-def utils_exceptions():
+@pytest.fixture(name="utils_exceptions")
+def fixture_utils_exceptions():
     return aws_exceptions
 
 

tests/unit/module_utils/retries/test_retry_wrapper.py

@@ -18,8 +18,8 @@
 import ansible_collections.amazon.aws.plugins.module_utils.retries as util_retries
 
 
-@pytest.fixture
-def fake_client():
+@pytest.fixture(name="fake_client")
+def fixture_fake_client():
     retryable_response = {"Error": {"Code": "RequestLimitExceeded", "Message": "Something went wrong"}}
     retryable_exception = botocore.exceptions.ClientError(retryable_response, "fail_retryable")
     not_retryable_response = {"Error": {"Code": "AnotherProblem", "Message": "Something went wrong"}}
@@ -35,8 +35,8 @@ def fake_client():
     return client
 
 
-@pytest.fixture
-def quick_backoff():
+@pytest.fixture(name="quick_backoff")
+def fixture_quick_backoff():
     # Because RetryingBotoClientWrapper will wrap resources using the this decorator,
     # we're going to rely on AWSRetry.jittered_backoff rather than trying to mock out
     # a decorator use a really short delay to keep the tests quick, and we only need