Minor Unit test and PEP8 sanity fixes (Ansible 2.16) (#1846) (#1850)

[PR #1846/94bfa6f1 backport][stable-7] Minor Unit test and PEP8 sanity fixes (Ansible 2.16) This is a backport of PR #1846 as merged into main (94bfa6f). SUMMARY Addition of Ansible 2.16 has surfaced some new PEP8 issues: https://github.com/ansible-collections/amazon.aws/actions/runs/6822667301/job/18555114862?pr=1845 As well as exposing a 'broken' unit test (the test it was using didn't actually exist, it was only passing because we're using MagicMock which added a stub. Old versions of Python and Ansible have also been dropped by the upstream test, we don't need the include/exclude overrides any more. The latest versions of Python have also picked up on a bad unit test in ec2_ami_info. ISSUE TYPE Bugfix Pull Request COMPONENT NAME rds_instance_snapshot autoscaling_group ec2_ami_info ADDITIONAL INFORMATION Reviewed-by: Mark Chappell
ansible-collections · Nov 14, 2023 · b487b66 · b487b66
1 parent f03c639
commit b487b66
Show file tree

Hide file tree

Showing 7 changed files with 28 additions and 103 deletions.
diff --git a/.github/workflows/sanity.yml b/.github/workflows/sanity.yml
@@ -6,67 +6,3 @@ on: [workflow_call]  # allow this workflow to be called from other workflows
 jobs:
   sanity:
     uses: ansible-network/github_actions/.github/workflows/sanity.yml@main
-    with:
-      matrix_include: "[]"
-      matrix_exclude: >-
-          [
-            {
-              "ansible-version": "stable-2.9"
-            },
-            {
-              "ansible-version": "stable-2.12",
-              "python-version": "3.7"
-            },
-            {
-              "ansible-version": "stable-2.12",
-              "python-version": "3.11"
-            },
-            {
-              "ansible-version": "stable-2.13",
-              "python-version": "3.7"
-            },
-            {
-              "ansible-version": "stable-2.13",
-              "python-version": "3.11"
-            },
-            {
-              "ansible-version": "stable-2.14",
-              "python-version": "3.7"
-            },
-            {
-              "ansible-version": "stable-2.14",
-              "python-version": "3.8"
-            },
-            {
-              "ansible-version": "stable-2.15",
-              "python-version": "3.7"
-            },
-            {
-              "ansible-version": "stable-2.15",
-              "python-version": "3.8"
-            },
-            {
-              "ansible-version": "milestone",
-              "python-version": "3.7"
-            },
-            {
-              "ansible-version": "milestone",
-              "python-version": "3.8"
-            },
-            {
-              "ansible-version": "milestone",
-              "python-version": "3.9"
-            },
-            {
-              "ansible-version": "devel",
-              "python-version": "3.7"
-            },
-            {
-              "ansible-version": "devel",
-              "python-version": "3.8"
-            },
-            {
-              "ansible-version": "devel",
-              "python-version": "3.9"
-            }
-          ]
diff --git a/changelogs/fragments/20231110-sanity.yml b/changelogs/fragments/20231110-sanity.yml
@@ -0,0 +1,4 @@
+minor_changes:
+- autoscaling_group - minor PEP8 whitespace sanity fixes (https://github.com/ansible-collections/amazon.aws/pull/1846).
+- rds_instance_snapshot - minor PEP8 whitespace sanity fixes (https://github.com/ansible-collections/amazon.aws/pull/1846).
+- ec2_ami_info - simplify parameters to ``get_image_attribute`` to only pass ID of image (https://github.com/ansible-collections/amazon.aws/pull/1846).
diff --git a/plugins/modules/autoscaling_group.py b/plugins/modules/autoscaling_group.py
@@ -1675,15 +1675,15 @@ def get_instances_by_launch_config(props, lc_check, initial_instances):
                 old_instances.append(i)
 
     else:
-        module.debug(f"Comparing initial instances with current: {*initial_instances,}")
+        module.debug(f"Comparing initial instances with current: {*initial_instances, }")
         for i in props["instances"]:
             if i not in initial_instances:
                 new_instances.append(i)
             else:
                 old_instances.append(i)
 
-    module.debug(f"New instances: {len(new_instances)}, {*new_instances,}")
-    module.debug(f"Old instances: {len(old_instances)}, {*old_instances,}")
+    module.debug(f"New instances: {len(new_instances)}, {*new_instances, }")
+    module.debug(f"Old instances: {len(old_instances)}, {*old_instances, }")
 
     return new_instances, old_instances
 
@@ -1702,15 +1702,15 @@ def get_instances_by_launch_template(props, lt_check, initial_instances):
             else:
                 old_instances.append(i)
     else:
-        module.debug(f"Comparing initial instances with current: {*initial_instances,}")
+        module.debug(f"Comparing initial instances with current: {*initial_instances, }")
         for i in props["instances"]:
             if i not in initial_instances:
                 new_instances.append(i)
             else:
                 old_instances.append(i)
 
-    module.debug(f"New instances: {len(new_instances)}, {*new_instances,}")
-    module.debug(f"Old instances: {len(old_instances)}, {*old_instances,}")
+    module.debug(f"New instances: {len(new_instances)}, {*new_instances, }")
+    module.debug(f"Old instances: {len(old_instances)}, {*old_instances, }")
 
     return new_instances, old_instances
 
@@ -1775,9 +1775,9 @@ def terminate_batch(connection, replace_instances, initial_instances, leftovers=
     instances_to_terminate = list_purgeable_instances(props, lc_check, lt_check, replace_instances, initial_instances)
 
     module.debug(f"new instances needed: {num_new_inst_needed}")
-    module.debug(f"new instances: {*new_instances,}")
-    module.debug(f"old instances: {*old_instances,}")
-    module.debug(f"batch instances: {*instances_to_terminate,}")
+    module.debug(f"new instances: {*new_instances, }")
+    module.debug(f"old instances: {*old_instances, }")
+    module.debug(f"batch instances: {*instances_to_terminate, }")
 
     if num_new_inst_needed == 0:
         decrement_capacity = True

diff --git a/plugins/modules/ec2_ami_info.py b/plugins/modules/ec2_ami_info.py
@@ -260,10 +260,10 @@ def get_images(ec2_client, request_args):
     return images
 
 
-def get_image_attribute(ec2_client, image):
+def get_image_attribute(ec2_client, image_id):
     try:
         launch_permissions = ec2_client.describe_image_attribute(
-            aws_retry=True, Attribute="launchPermission", ImageId=image["image_id"]
+            aws_retry=True, Attribute="launchPermission", ImageId=image_id
         )
     except (ClientError, BotoCoreError) as err:
         raise AmiInfoFailure(err, "error describing image attribute")
@@ -276,9 +276,10 @@ def list_ec2_images(ec2_client, module, request_args):
 
     for image in images:
         try:
+            image_id = image["image_id"]
             image["tags"] = boto3_tag_list_to_ansible_dict(image.get("tags", []))
             if module.params.get("describe_image_attributes"):
-                launch_permissions = get_image_attribute(ec2_client, image).get("LaunchPermissions", [])
+                launch_permissions = get_image_attribute(ec2_client, image_id).get("LaunchPermissions", [])
                 image["launch_permissions"] = [camel_dict_to_snake_dict(perm) for perm in launch_permissions]
         except is_boto3_error_code("AuthFailure"):
             # describing launch permissions of images owned by others is not permitted, but shouldn't cause failures

diff --git a/plugins/modules/rds_instance_snapshot.py b/plugins/modules/rds_instance_snapshot.py
@@ -270,7 +270,7 @@ def get_parameters(parameters, method_name):
     required_options = get_boto3_client_method_parameters(client, method_name, required=True)
     if any(parameters.get(k) is None for k in required_options):
         method_description = get_rds_method_attribute(method_name, module).operation_description
-        module.fail_json(msg=f"To {method_description} requires the parameters: {*required_options,}")
+        module.fail_json(msg=f"To {method_description} requires the parameters: {*required_options, }")
     options = get_boto3_client_method_parameters(client, method_name)
     parameters = dict((k, v) for k, v in parameters.items() if k in options and v is not None)
 

diff --git a/tests/unit/module_utils/test_acm.py b/tests/unit/module_utils/test_acm.py
@@ -17,6 +17,7 @@
     # Handled by HAS_BOTO3
     pass
 
+
 from ansible_collections.amazon.aws.plugins.module_utils.acm import ACMServiceManager
 from ansible_collections.amazon.aws.plugins.module_utils.acm import acm_catch_boto_exception
 

diff --git a/tests/unit/plugins/modules/test_ec2_ami_info.py b/tests/unit/plugins/modules/test_ec2_ami_info.py
@@ -94,33 +94,13 @@ def test_get_image_attribute():
         "LaunchPermissions": [{"UserId": "1234567890"}, {"UserId": "0987654321"}],
     }
 
-    image = {
-        "architecture": "x86_64",
-        "blockDeviceMappings": [
-            {
-                "device_name": "/dev/sda1",
-                "ebs": {
-                    "delete_on_termination": "True",
-                    "encrypted": "False",
-                    "snapshot_id": "snap-0f00cba784af62428",
-                    "volume_size": 10,
-                    "volume_Type": "gp2",
-                },
-            }
-        ],
-        "image_id": "ami-1234567890",
-        "image_location": "1234567890/test-ami-uefi-boot",
-        "image_type": "machine",
-        "name": "test-ami-uefi-boot",
-        "owner_id": "1234567890",
-        "platform_details": "Linux/UNIX",
-    }
+    image_id = "ami-1234567890"
 
-    get_image_attribute_result = ec2_ami_info.get_image_attribute(ec2_client, image)
+    get_image_attribute_result = ec2_ami_info.get_image_attribute(ec2_client, image_id)
 
     ec2_client.describe_image_attribute.call_count == 1
     ec2_client.describe_image_attribute.assert_called_with(
-        aws_retry=True, Attribute="launchPermission", ImageId=image["image_id"]
+        aws_retry=True, Attribute="launchPermission", ImageId=image_id
     )
     assert len(get_image_attribute_result["LaunchPermissions"]) == 2
 
@@ -203,7 +183,10 @@ def test_list_ec2_images(m_get_images, m_get_image_attribute):
     m_get_images.assert_called_with(ec2_client, request_args)
 
     assert m_get_image_attribute.call_count == 2
-    assert m_get_image_attribute.has_calls([call(ec2_client, images[0])], [call(ec2_client, images[1])])
+    m_get_image_attribute.assert_has_calls(
+        [call(ec2_client, images[0]["image_id"])],
+        [call(ec2_client, images[1]["image_id"])],
+    )
 
     assert len(list_ec2_images_result) == 2
     assert list_ec2_images_result[0]["image_id"] == "ami-1234567890"
@@ -234,8 +217,8 @@ def test_api_failure_get_images(ec2_client):
 
 
 def test_api_failure_get_image_attribute(ec2_client):
-    image = {"image_id": "ami-1234567890"}
+    image_id = "ami-1234567890"
     ec2_client.describe_image_attribute.side_effect = a_boto_exception()
 
     with pytest.raises(ec2_ami_info.AmiInfoFailure):
-        ec2_ami_info.get_image_attribute(ec2_client, image)
+        ec2_ami_info.get_image_attribute(ec2_client, image_id)
-Original file line number
+Diff line change
@@ Expand Up / @@ -17,6 +17,7 @@ @@
         # Handled by HAS_BOTO3
         pass
     from ansible_collections.amazon.aws.plugins.module_utils.acm import ACMServiceManager
     from ansible_collections.amazon.aws.plugins.module_utils.acm import acm_catch_boto_exception
@@ Expand Down @@