From dc0247e063d7790f5702500a017bfdb2a4d98afc Mon Sep 17 00:00:00 2001 From: Joseph Torcasso Date: Mon, 6 Jun 2022 22:41:48 -0400 Subject: [PATCH 1/6] initial go at adding docs to return values for _info modules --- docs/docsite/rst/dev_guidelines.rst | 73 +++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/docs/docsite/rst/dev_guidelines.rst b/docs/docsite/rst/dev_guidelines.rst index 89a6bd5065c..05d971bfe9c 100644 --- a/docs/docsite/rst/dev_guidelines.rst +++ b/docs/docsite/rst/dev_guidelines.rst @@ -556,6 +556,79 @@ E.g. if boto3 returns a value called 'SecretAccessKey' do not change it to 'Acce .. _ansible_collections.amazon.aws.docsite.dev_policies: +Info modules +------------ + +Info modules that can return information on multiple resources should return a singular value representing a list of dictionaries, +with each dictionary containing information about that particular resource (i.e. ``security_groups`` in ``ec2_group_info``). +In cases where the _info module only returns information on a singular resource (i.e. ``ec2_tag_info``), +a singular dictionary should be returned as opposed to a list of dictionaries. +In cases where the _info module returns no instances, and empty list '[]' should be returned. +Keys in the returned dictionaries should follow the guidelines above and use snake_case. +If a return value can be used as a parameter for its corresponding main module, the key should match either +the parameter name itself, or an alias of that parameter. The following is an example of improper usage of an info module with its respective main module: + +.. code-block:: yaml + + "security_groups": [ + { + "description": "Created by ansible integration tests", + "group_id": "sg-050dba5c3520cba71", + "group_name": "ansible-test-87988625-unknown5c5f67f3ad09-icmp-1", + "ip_permissions": [ + { + "from_port": 3, + "ip_protocol": "icmp", + "ip_ranges": [ + { + "cidr_ip": "172.16.40.10/32" + }, + { + "cidr_ip": "10.0.0.0/8" + } + ], + "ipv6_ranges": [], + "prefix_list_ids": [], + "to_port": 8, + "user_id_group_pairs": [] + } + ], + "ip_permissions_egress": [ + { + "ip_protocol": "-1", + "ip_ranges": [ + { + "cidr_ip": "0.0.0.0/0" + } + ], + "ipv6_ranges": [], + "prefix_list_ids": [], + "user_id_group_pairs": [] + } + ], + "owner_id": "721066863947", + "tags": {}, + "vpc_id": "vpc-0cbc2380a326b8a0d" + } + ] + +The output above is what was returned by the ``ec2_group_info`` module. It correctly returns a list of dictionaries called ``security_groups``; however, +the parameter names do not match those of the ``ec2_group`` module. ``ec2_group`` requires ``name``, meanwhile this returns ``group_name``. + +Deprecating return values +------------------------- + +If changes need to be made to current return values, the new/"correct" keys should be returned **in addition to** the existing keys to preserve +compability with existing playbooks. A deprecation should be added to the return values being replaced, initially placed 2 years out. +For example: + +.. code-block:: python + + # Deprecate old `iam_user` return key to be replaced by `user` introduced on 2022-05-01 + module.deprecate("The 'iam_user' return key is deprecated and will be replaced by 'user'. Both values are returned for now.", + date='2024-05-01', collection_name='community.aws') + + Dealing with IAM JSON policy ============================ From 5c01c3280c44cce536016cd3a2ea3608a94cc582 Mon Sep 17 00:00:00 2001 From: Joseph Torcasso Date: Thu, 9 Jun 2022 17:58:30 -0400 Subject: [PATCH 2/6] add correct example and loosen deprecation date --- docs/docsite/rst/dev_guidelines.rst | 130 ++++++++++++++++++---------- 1 file changed, 83 insertions(+), 47 deletions(-) diff --git a/docs/docsite/rst/dev_guidelines.rst b/docs/docsite/rst/dev_guidelines.rst index 05d971bfe9c..4dc892c7e8c 100644 --- a/docs/docsite/rst/dev_guidelines.rst +++ b/docs/docsite/rst/dev_guidelines.rst @@ -540,94 +540,130 @@ should return in the module. As well as information related to the call itself, some response metadata. It is OK to return this to the user as well as they may find it useful. Boto3 returns all values CamelCased. Ansible follows Python standards for variable names and uses -snake_case. There is a helper function in module_utils/ec2.py called ``camel_dict_to_snake_dict`` +snake_case. There is a helper function in module_utils/ec2.py called ``camel_dict_to_snake_dict`` that allows you to easily convert the boto3 response to snake_case. You should use this helper function and avoid changing the names of values returned by Boto3. E.g. if boto3 returns a value called 'SecretAccessKey' do not change it to 'AccessKey'. +There is an optional parameter, ``ignore_list``, which is used to avoid converting a sub-tree +of a dict. This is particularly important for tags, where keys are case-sensitive. We +convert the 'Tags' key but nothing below. + .. code-block:: python # Make a call to AWS result = connection.aws_call() - # Return the result to the user - module.exit_json(changed=True, **camel_dict_to_snake_dict(result)) + # Return the result to the user without modifying tags + module.exit_json(changed=True, **camel_dict_to_snake_dict(result, ignore_list=['Tags'])) -.. _ansible_collections.amazon.aws.docsite.dev_policies: +Tags +---- + +Tags should be returned as a dictionary of key: value pairs, with each key being the tag's +key and value being the tag's value. It should be noted, however, that boto3 returns tags +as a list of dictionaries. + +There is a helper function in module_utils/ec2.py called ``boto3_tag_list_to_ansible_dict`` +(discussed in detail below in the "Helper Functions" section) that allows for an easy conversion +from boto3's returned tag list to the desired dictionary of tags to be returned by the module. + +Below is a full example of getting the result of an AWS call and returning the expected values: + +.. code-block:: python + + # Make a call to AWS + result = connection.aws_call() + + # Make result snake_case without modifying tags + snaked_result = camel_dict_to_snake_dict(result, ignore_list=['Tags']) + + # Convert boto3 list of dict tags to just a dict of tags + snaked_result['tags'] = boto3_tag_list_to_ansible_dict(result.get('tags', [])) + + # Return the result to the user + module.exit_json(changed=True, **snaked_result) Info modules ------------ -Info modules that can return information on multiple resources should return a singular value representing a list of dictionaries, -with each dictionary containing information about that particular resource (i.e. ``security_groups`` in ``ec2_group_info``). -In cases where the _info module only returns information on a singular resource (i.e. ``ec2_tag_info``), -a singular dictionary should be returned as opposed to a list of dictionaries. +Info modules that can return information on multiple resources should return a list of +dictionaries, with each dictionary containing information about that particular resource +(i.e. ``security_groups`` in ``ec2_group_info``). + +In cases where the _info module only returns information on a singular resource +(i.e. ``ec2_tag_info``), a singular dictionary should be returned as opposed to a list +of dictionaries. + In cases where the _info module returns no instances, and empty list '[]' should be returned. + Keys in the returned dictionaries should follow the guidelines above and use snake_case. -If a return value can be used as a parameter for its corresponding main module, the key should match either -the parameter name itself, or an alias of that parameter. The following is an example of improper usage of an info module with its respective main module: +If a return value can be used as a parameter for its corresponding main module, the key should +match either the parameter name itself, or an alias of that parameter. + +The following is an example of improper usage of a sample info module with its respective main module: .. code-block:: yaml - "security_groups": [ + "security_groups": { { "description": "Created by ansible integration tests", "group_id": "sg-050dba5c3520cba71", "group_name": "ansible-test-87988625-unknown5c5f67f3ad09-icmp-1", - "ip_permissions": [ - { - "from_port": 3, - "ip_protocol": "icmp", - "ip_ranges": [ - { - "cidr_ip": "172.16.40.10/32" - }, - { - "cidr_ip": "10.0.0.0/8" - } - ], - "ipv6_ranges": [], - "prefix_list_ids": [], - "to_port": 8, - "user_id_group_pairs": [] - } - ], - "ip_permissions_egress": [ + "ip_permissions": [], + "ip_permissions_egress": [], + "owner_id": "721066863947", + "tags": [ { - "ip_protocol": "-1", - "ip_ranges": [ - { - "cidr_ip": "0.0.0.0/0" - } - ], - "ipv6_ranges": [], - "prefix_list_ids": [], - "user_id_group_pairs": [] - } + "Key": "Tag_One" + "Value": "Tag_One_Value" + }, ], + "vpc_id": "vpc-0cbc2380a326b8a0d" + } + } + +The sample output above shows a few mistakes in the sample security group info module: +* ``security_groups`` is a dict of dict, not a list of dicts. +* ``tags`` appears to be directly returned from boto3, since they're a list of dicts. + +The following is what the sample output would look like, with the mistakes corrected. + +.. code-block:: yaml + + "security_groups": [ + { + "description": "Created by ansible integration tests", + "group_id": "sg-050dba5c3520cba71", + "group_name": "ansible-test-87988625-unknown5c5f67f3ad09-icmp-1", + "ip_permissions": [], + "ip_permissions_egress": [], "owner_id": "721066863947", - "tags": {}, + "tags": { + "Tag_One": "Tag_One_Value", + }, "vpc_id": "vpc-0cbc2380a326b8a0d" } ] -The output above is what was returned by the ``ec2_group_info`` module. It correctly returns a list of dictionaries called ``security_groups``; however, -the parameter names do not match those of the ``ec2_group`` module. ``ec2_group`` requires ``name``, meanwhile this returns ``group_name``. - Deprecating return values ------------------------- -If changes need to be made to current return values, the new/"correct" keys should be returned **in addition to** the existing keys to preserve -compability with existing playbooks. A deprecation should be added to the return values being replaced, initially placed 2 years out. +If changes need to be made to current return values, the new/"correct" keys should be +returned **in addition to** the existing keys to preserve compability with existing +playbooks. A deprecation should be added to the return values being replaced, initially +placed at least 2 years out, on the 1st of a month. + For example: .. code-block:: python - # Deprecate old `iam_user` return key to be replaced by `user` introduced on 2022-05-01 + # Deprecate old `iam_user` return key to be replaced by `user` introduced on 2022-04-10 module.deprecate("The 'iam_user' return key is deprecated and will be replaced by 'user'. Both values are returned for now.", date='2024-05-01', collection_name='community.aws') +.. _ansible_collections.amazon.aws.docsite.dev_policies: Dealing with IAM JSON policy ============================ From 0a5c75bfba07cebe0655ebfbf53c833155a4828e Mon Sep 17 00:00:00 2001 From: Joseph Torcasso <87090265+jatorcasso@users.noreply.github.com> Date: Fri, 10 Jun 2022 11:51:24 -0400 Subject: [PATCH 3/6] Apply suggestions from code review Co-authored-by: Mark Chappell --- docs/docsite/rst/dev_guidelines.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/docsite/rst/dev_guidelines.rst b/docs/docsite/rst/dev_guidelines.rst index 4dc892c7e8c..2410d2bc986 100644 --- a/docs/docsite/rst/dev_guidelines.rst +++ b/docs/docsite/rst/dev_guidelines.rst @@ -539,8 +539,8 @@ When you make a call using boto3, you will probably get back some useful informa should return in the module. As well as information related to the call itself, you will also have some response metadata. It is OK to return this to the user as well as they may find it useful. -Boto3 returns all values CamelCased. Ansible follows Python standards for variable names and uses -snake_case. There is a helper function in module_utils/ec2.py called ``camel_dict_to_snake_dict`` +Boto3 commonly returns all keys CamelCased. Ansible follows Python standards for variable names and uses +snake_case. There is a commonly used helper function ``ansible.module_utils.common.dict_transformations.camel_dict_to_snake_dict`` that allows you to easily convert the boto3 response to snake_case. You should use this helper function and avoid changing the names of values returned by Boto3. @@ -562,7 +562,7 @@ Tags ---- Tags should be returned as a dictionary of key: value pairs, with each key being the tag's -key and value being the tag's value. It should be noted, however, that boto3 returns tags +key and value being the tag's value. It should be noted, however, that boto3 often returns tags as a list of dictionaries. There is a helper function in module_utils/ec2.py called ``boto3_tag_list_to_ansible_dict`` @@ -596,7 +596,7 @@ In cases where the _info module only returns information on a singular resource (i.e. ``ec2_tag_info``), a singular dictionary should be returned as opposed to a list of dictionaries. -In cases where the _info module returns no instances, and empty list '[]' should be returned. +In cases where the _info module returns no instances, an empty list '[]' should be returned. Keys in the returned dictionaries should follow the guidelines above and use snake_case. If a return value can be used as a parameter for its corresponding main module, the key should From 4a5e6b58d732c46d1c4ecb507a80344a25260cea Mon Sep 17 00:00:00 2001 From: Joseph Torcasso Date: Sun, 26 Jun 2022 20:02:52 -0400 Subject: [PATCH 4/6] remove confusing sentence for tags --- docs/docsite/rst/dev_guidelines.rst | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/docsite/rst/dev_guidelines.rst b/docs/docsite/rst/dev_guidelines.rst index 2410d2bc986..ad53a61dd3c 100644 --- a/docs/docsite/rst/dev_guidelines.rst +++ b/docs/docsite/rst/dev_guidelines.rst @@ -547,8 +547,7 @@ You should use this helper function and avoid changing the names of values retur E.g. if boto3 returns a value called 'SecretAccessKey' do not change it to 'AccessKey'. There is an optional parameter, ``ignore_list``, which is used to avoid converting a sub-tree -of a dict. This is particularly important for tags, where keys are case-sensitive. We -convert the 'Tags' key but nothing below. +of a dict. This is particularly useful for tags, where keys are case-sensitive. .. code-block:: python From 846f0d17e842d9e66272ffb73b09fd94c8201d39 Mon Sep 17 00:00:00 2001 From: Joseph Torcasso Date: Wed, 29 Jun 2022 10:37:29 -0400 Subject: [PATCH 5/6] more info on return vals --- docs/docsite/rst/dev_guidelines.rst | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/docs/docsite/rst/dev_guidelines.rst b/docs/docsite/rst/dev_guidelines.rst index ad53a61dd3c..9533bf82804 100644 --- a/docs/docsite/rst/dev_guidelines.rst +++ b/docs/docsite/rst/dev_guidelines.rst @@ -551,11 +551,18 @@ of a dict. This is particularly useful for tags, where keys are case-sensitive. .. code-block:: python - # Make a call to AWS - result = connection.aws_call() + # Make a call to AWS + resource = connection.aws_call() + + # Convert resource response to snake_case + snaked_resource = camel_dict_to_snake_dict(resource, ignore_list=['Tags']) + + # Return the resource details to the user without modifying tags + module.exit_json(changed=True, some_resource=snaked_resource) - # Return the result to the user without modifying tags - module.exit_json(changed=True, **camel_dict_to_snake_dict(result, ignore_list=['Tags'])) +Note: The returned key representing the details of the specific resource (``some_resource`` above) +should be a sensible approximation of the resource name. For example, ``volume`` for ``ec2_vol``, +``volumes`` for ``ec2_vol_info``. Tags ---- From 209756b29cd6287886bbba6ee12be0461ff32ab7 Mon Sep 17 00:00:00 2001 From: Joseph Torcasso Date: Tue, 5 Jul 2022 19:14:05 -0400 Subject: [PATCH 6/6] update with review --- docs/docsite/rst/dev_guidelines.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/docsite/rst/dev_guidelines.rst b/docs/docsite/rst/dev_guidelines.rst index 9533bf82804..31f9dfbfe5c 100644 --- a/docs/docsite/rst/dev_guidelines.rst +++ b/docs/docsite/rst/dev_guidelines.rst @@ -539,9 +539,9 @@ When you make a call using boto3, you will probably get back some useful informa should return in the module. As well as information related to the call itself, you will also have some response metadata. It is OK to return this to the user as well as they may find it useful. -Boto3 commonly returns all keys CamelCased. Ansible follows Python standards for variable names and uses -snake_case. There is a commonly used helper function ``ansible.module_utils.common.dict_transformations.camel_dict_to_snake_dict`` -that allows you to easily convert the boto3 response to snake_case. +Boto3 returns most keys in CamelCase. Ansible adopts python standards for naming variables and usage. +There is a useful helper function called ``camel_dict_to_snake_dict`` that allows for an easy conversion +of the boto3 response to snake_case. It resides in ``module_utils/common/dict_transformations``. You should use this helper function and avoid changing the names of values returned by Boto3. E.g. if boto3 returns a value called 'SecretAccessKey' do not change it to 'AccessKey'.