Address review comments

ansible-collections · Oct 9, 2024 · 736a31a · 736a31a
1 parent 8a96512
commit 736a31a
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 61 deletions.
diff --git a/plugins/modules/ec2_placement_group.py b/plugins/modules/ec2_placement_group.py
@@ -116,16 +116,16 @@
 from ansible_collections.amazon.aws.plugins.module_utils.ec2 import create_ec2_placement_group
 from ansible_collections.amazon.aws.plugins.module_utils.ec2 import delete_ec2_placement_group
 from ansible_collections.amazon.aws.plugins.module_utils.ec2 import describe_ec2_placement_groups
-from ansible_collections.amazon.aws.plugins.module_utils.modules import AnsibleAWSModule
 from ansible_collections.amazon.aws.plugins.module_utils.tagging import boto3_tag_list_to_ansible_dict
 from ansible_collections.amazon.aws.plugins.module_utils.tagging import boto3_tag_specifications
 
+from ansible_collections.community.aws.plugins.module_utils.modules import AnsibleCommunityAWSModule as AnsibleAWSModule
 
-def search_placement_group(connection, module: AnsibleAWSModule) -> Dict[str, Any]:
+
+def search_placement_group(connection, name: str) -> Dict[str, Any]:
     """
     Check if a placement group exists.
     """
-    name = module.params.get("name")
     response = describe_ec2_placement_groups(connection, Filters=[{"Name": "group-name", "Values": [name]}])
 
     if len(response) != 1:
@@ -170,26 +170,26 @@ def create_placement_group(connection, module: AnsibleAWSModule) -> None:
         params["TagSpecifications"] = boto3_tag_specifications(tags, types=["placement-group"])
     if partition_count:
         params["PartitionCount"] = partition_count
-    params["DryRun"] = module.check_mode
-
-    response = create_ec2_placement_group(connection, **params)
-    if response.get("boto3_error_code") == "DryRunOperation":
+    if module.check_mode:
         module.exit_json(
             changed=True,
             placement_group={
                 "name": name,
-                "state": "DryRun",
                 "strategy": strategy,
                 "tags": tags,
             },
+            msg="EC2 placement group would be created if not in check mode"
         )
+
+    response = create_ec2_placement_group(connection, **params)
     module.exit_json(changed=True, placement_group=get_placement_group_information(connection, name))
 
 
 def delete_placement_group(connection, module: AnsibleAWSModule) -> None:
+    if module.check_mode:
+        module.exit_json(changed=True, msg="VPC would be deleted if not in check mode")
     name = module.params.get("name")
-
-    delete_ec2_placement_group(connection, name, module.check_mode)
+    delete_ec2_placement_group(connection, name)
     module.exit_json(changed=True)
 
 
@@ -207,9 +207,10 @@ def main():
     connection = module.client("ec2")
 
     state = module.params.get("state")
+    name = module.params.get("name")
+    placement_group = search_placement_group(connection, name)
 
     if state == "present":
-        placement_group = search_placement_group(connection, module)
         if placement_group is None:
             create_placement_group(connection, module)
         else:
@@ -223,7 +224,6 @@ def main():
                 )
 
     elif state == "absent":
-        placement_group = search_placement_group(connection, module)
         if placement_group is None:
             module.exit_json(changed=False)
         else:

diff --git a/plugins/modules/ec2_placement_group_info.py b/plugins/modules/ec2_placement_group_info.py
@@ -79,26 +79,23 @@
 
 from typing import Any
 from typing import Dict
+from typing import List
 
 from ansible_collections.amazon.aws.plugins.module_utils.ec2 import describe_ec2_placement_groups
 from ansible_collections.amazon.aws.plugins.module_utils.modules import AnsibleAWSModule
 from ansible_collections.amazon.aws.plugins.module_utils.tagging import boto3_tag_list_to_ansible_dict
 
 
-def get_placement_groups_details(connection, module: AnsibleAWSModule) -> Dict[str, Any]:
-    names = module.params.get("names")
+def get_placement_groups_details(connection, names: List) -> Dict[str, Any]:
+    params = {}
     if len(names) > 0:
-        response = describe_ec2_placement_groups(
-            connection,
-            Filters=[
-                {
-                    "Name": "group-name",
-                    "Values": names,
-                }
-            ],
-        )
-    else:
-        response = describe_ec2_placement_groups(connection)
+        params["Filters"] = [
+            {
+                "Name": "group-name",
+                "Values": names,
+            }
+        ]
+    response = describe_ec2_placement_groups(connection, **params)
 
     results = []
     for placement_group in response:
@@ -124,8 +121,9 @@ def main():
     )
 
     connection = module.client("ec2")
+    names =  module.params.get("names")
 
-    placement_groups = get_placement_groups_details(connection, module)
+    placement_groups = get_placement_groups_details(connection, names)
     module.exit_json(changed=False, placement_groups=placement_groups)
 
 

diff --git a/tests/integration/targets/ec2_placement_group/tasks/main.yml b/tests/integration/targets/ec2_placement_group/tasks/main.yml
@@ -27,8 +27,6 @@
         that:
           - pg_1_create_check_mode is changed
           - pg_1_create_check_mode.placement_group.name == resource_prefix ~ '-pg1'
-          - pg_1_create_check_mode.placement_group.state == "DryRun"
-          - '"ec2:CreatePlacementGroup" in pg_1_create_check_mode.resource_actions'
 
     - name: Create a placement group 1
       community.aws.ec2_placement_group:
@@ -89,7 +87,6 @@
           - pg_1_create_check_mode_idem is not changed
           - pg_1_create_check_mode_idem.placement_group.name == resource_prefix ~ '-pg1'
           - pg_1_create_check_mode_idem.placement_group.state == "available"
-          - '"ec2:CreatePlacementGroup" not in pg_1_create_check_mode_idem.resource_actions'
 
     - name: Create a placement group 2 - check_mode
       community.aws.ec2_placement_group:
@@ -104,8 +101,6 @@
         that:
           - pg_2_create_check_mode is changed
           - pg_2_create_check_mode.placement_group.name == resource_prefix ~ '-pg2'
-          - pg_2_create_check_mode.placement_group.state == "DryRun"
-          - '"ec2:CreatePlacementGroup" in pg_2_create_check_mode.resource_actions'
 
     - name: Create a placement group 2 with spread strategy
       community.aws.ec2_placement_group:
@@ -169,7 +164,6 @@
           - pg_2_create_check_mode_idem is not changed
           - pg_2_create_check_mode_idem.placement_group.name == resource_prefix ~ '-pg2'
           - pg_2_create_check_mode_idem.placement_group.state == "available"
-          - '"ec2:CreatePlacementGroup" not in pg_2_create_check_mode_idem.resource_actions'
 
     - name: Create a placement group 3 - check_mode
       community.aws.ec2_placement_group:
@@ -185,8 +179,6 @@
         that:
           - pg_3_create_check_mode is changed
           - pg_3_create_check_mode.placement_group.name == resource_prefix ~ '-pg3'
-          - pg_3_create_check_mode.placement_group.state == "DryRun"
-          - '"ec2:CreatePlacementGroup" in pg_3_create_check_mode.resource_actions'
 
     - name: Create a placement group 3 with Partition strategy
       community.aws.ec2_placement_group:
@@ -254,7 +246,6 @@
           - pg_3_create_check_mode_idem is not changed
           - pg_3_create_check_mode_idem.placement_group.name == resource_prefix ~ '-pg3'
           - pg_3_create_check_mode_idem.placement_group.state == "available"
-          - '"ec2:CreatePlacementGroup" not in pg_3_create_check_mode_idem.resource_actions'
 
     - name: Create a placement group 4 with tags - check_mode
       community.aws.ec2_placement_group:
@@ -272,10 +263,8 @@
         that:
           - pg_4_create_check_mode is changed
           - pg_4_create_check_mode.placement_group.name == resource_prefix ~ '-pg4'
-          - pg_4_create_check_mode.placement_group.state == "DryRun"
           - pg_4_create_check_mode.placement_group.tags.foo == "test1"
           - pg_4_create_check_mode.placement_group.tags.bar == "test2"
-          - '"ec2:CreatePlacementGroup" in pg_4_create_check_mode.resource_actions'
 
     - name: Create a placement group 4 with tags
       community.aws.ec2_placement_group:
@@ -358,17 +347,13 @@
           - pg_4_create_check_mode_idem.placement_group.strategy == "cluster"
           - pg_4_create_check_mode_idem.placement_group.tags.foo == "test1"
           - pg_4_create_check_mode_idem.placement_group.tags.bar == "test2"
-          - '"ec2:CreatePlacementGroup" not in pg_4_create_check_mode_idem.resource_actions'
 
     - name: List all placement groups.
       community.aws.ec2_placement_group_info:
       register: all_ec2_placement_groups
 
 # Delete Placement Group ==========================================
 
-    # On using check_mode for delete placement group operation
-    # If operation would have succeeded, the error response is DryRunOperation.
-    # Otherwise, it is UnauthorizedOperation .
     - name: Delete a placement group 1 - check_mode
       community.aws.ec2_placement_group:
         name: '{{ resource_prefix }}-pg1'
@@ -377,12 +362,10 @@
       register: pg_1_delete_check_mode
       ignore_errors: true
 
-    - name: Assert that placement group exists (check mode)
+    - name: Assert check mode (check mode)
       ansible.builtin.assert:
         that:
-          - pg_1_delete_check_mode is not changed
-          - pg_1_delete_check_mode.error.code == 'DryRunOperation'
-          - '"ec2:DeletePlacementGroup" in pg_1_delete_check_mode.resource_actions'
+          - pg_1_delete_check_mode is changed
 
     - name: Delete a placement group 1
       community.aws.ec2_placement_group:
@@ -420,7 +403,6 @@
       ansible.builtin.assert:
         that:
           - pg_1_delete_check_mode_idem is not changed
-          - '"ec2:DeletePlacementGroup" not in pg_1_delete_check_mode_idem.resource_actions'
 
     - name: Delete a placement group 2 - check_mode
       community.aws.ec2_placement_group:
@@ -430,12 +412,10 @@
       register: pg_2_delete_check_mode
       ignore_errors: true
 
-    - name: Assert that there is no change
+    - name: Assert that check mode is successful
       ansible.builtin.assert:
         that:
-          - pg_2_delete_check_mode is not changed
-          - pg_2_delete_check_mode.error.code == 'DryRunOperation'
-          - '"ec2:DeletePlacementGroup" in pg_2_delete_check_mode.resource_actions'
+          - pg_2_delete_check_mode is changed
 
     - name: Delete a placement group 2
       community.aws.ec2_placement_group:
@@ -473,7 +453,6 @@
       ansible.builtin.assert:
         that:
           - pg_2_delete_check_mode_idem is not changed
-          - '"ec2:DeletePlacementGroup" not in pg_2_delete_check_mode_idem.resource_actions'
 
     - name: Delete a placement group 3 - check_mode
       community.aws.ec2_placement_group:
@@ -483,12 +462,10 @@
       register: pg_3_delete_check_mode
       ignore_errors: true
 
-    - name: Assert that there is no change
+    - name: Assert that there is change - check mode
       ansible.builtin.assert:
         that:
-          - pg_3_delete_check_mode is not changed
-          - pg_3_delete_check_mode.error.code == 'DryRunOperation'
-          - '"ec2:DeletePlacementGroup" in pg_3_delete_check_mode.resource_actions'
+          - pg_3_delete_check_mode is changed
 
     - name: Delete a placement group 3
       community.aws.ec2_placement_group:
@@ -526,7 +503,6 @@
       ansible.builtin.assert:
         that:
           - pg_3_delete_check_mode_idem is not changed
-          - '"ec2:DeletePlacementGroup" not in pg_3_delete_check_mode_idem.resource_actions'
 
     - name: Delete a placement group 4 - check_mode
       community.aws.ec2_placement_group:
@@ -536,12 +512,10 @@
       register: pg_4_delete_check_mode
       ignore_errors: true
 
-    - name: Assert that there is no change
+    - name: Assert that there is change check mode
       ansible.builtin.assert:
         that:
-          - pg_4_delete_check_mode is not changed
-          - pg_4_delete_check_mode.error.code == 'DryRunOperation'
-          - '"ec2:DeletePlacementGroup" in pg_4_delete_check_mode.resource_actions'
+          - pg_4_delete_check_mode is changed
 
 
     - name: Delete a placement group 4
@@ -580,7 +554,6 @@
       ansible.builtin.assert:
         that:
           - pg_4_delete_check_mode_idem is not changed
-          - '"ec2:DeletePlacementGroup" not in pg_4_delete_check_mode_idem.resource_actions'
 
   always: