From 5d080fe1d78d043e988b669be8184330948cc3f1 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Sat, 28 May 2022 15:10:57 +0200 Subject: [PATCH 1/3] Tagging - add docs fragment and update guidelines --- docs/docsite/rst/dev_guidelines.rst | 54 +++++++++++++++++++++----- plugins/doc_fragments/tags.py | 59 +++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 9 deletions(-) create mode 100644 plugins/doc_fragments/tags.py diff --git a/docs/docsite/rst/dev_guidelines.rst b/docs/docsite/rst/dev_guidelines.rst index f23324b1afe..5296dc6e9ab 100644 --- a/docs/docsite/rst/dev_guidelines.rst +++ b/docs/docsite/rst/dev_guidelines.rst @@ -172,6 +172,20 @@ authentication parameters. To do the same for your new module, add an entry for aws_module_name: - aws +Module behavior +--------------- + +To reduce the chance of breaking changes occurring when new features are added, +when a parameter is not explicitly set in a task, the module should avoid +modifying the resource attribute. + +By convention, when a parameter is explicitly set in a task, the module should +set the resource attribute to match what was set in the task. In some cases, +such as tags or associations, it can be helpful to add an additional parameter +which changes this behavior to add the new setting to the attribute rather than +replacing the current attributes. Such a parameter should default to the +replacive rather than additive behavior. + .. _ansible_collections.amazon.aws.docsite.dev_module_connection: Connecting to AWS @@ -526,18 +540,40 @@ and returns True if they are different. Dealing with tags ================= -AWS has a concept of resource tags. Usually the boto3 API has separate calls for tagging and -untagging a resource. For example, the ec2 API has a create_tags and delete_tags call. +AWS has a concept of resource tags. Usually the boto3 API has separate calls +for tagging and untagging a resource. For example, the EC2 API has +``create_tags`` and ``delete_tags`` calls. + +When adding tagging support, Ansible AWS modules should add a ``tags`` parameter +that defaults to ``None`` and a ``purge_tags`` parameter that defaults to +``True``. + + +.. code-block:: python + + argument_spec.update( + dict( + tags=dict(type='dict', required=False, default=None), + purge_tags=dict(type='bool', required=False, default=True), + ) + ) + +When the ``purge_tags`` parameter is set to ``True`` **and** the ``tags`` +parameter is explicitly set in the task, then any tags not explicitly set in +``tags`` should be removed. + +If the ``tags`` parameter is not set then tags should not be modified, even if +``purge_tags`` is set to ``True``. This means that removing all tags requires +``tags`` be explicitly set to an empty dictionary ``{}`` in the Ansible task. -It is common practice in Ansible AWS modules to have a ``purge_tags`` parameter that defaults to -true. -The ``purge_tags`` parameter means that existing tags will be deleted if they are not specified by -the Ansible task. +There is a helper function ``compare_aws_tags`` to ease dealing with tags. It +compares two dictionaries, the current tags and the desired tags, and returns +the tags to set and the tags to delete. See the Helper function section below +for more detail. -There is a helper function ``compare_aws_tags`` to ease dealing with tags. It can compare two dicts -and return the tags to set and the tags to delete. See the Helper function section below for more -detail. +There is also a documentation fragment ``amazon.aws.tags`` which should be +included when adding tagging support. .. _ansible_collections.amazon.aws.docsite.dev_helpers: diff --git a/plugins/doc_fragments/tags.py b/plugins/doc_fragments/tags.py new file mode 100644 index 00000000000..869138445aa --- /dev/null +++ b/plugins/doc_fragments/tags.py @@ -0,0 +1,59 @@ +# -*- coding: utf-8 -*- + +# Copyright: (c) 2022, Ansible Project +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) + +from __future__ import (absolute_import, division, print_function) +__metaclass__ = type + + +class ModuleDocFragment(object): + + # Minimum requirements for the collection + DOCUMENTATION = r''' +options: + tags: + description: + - A dictionary representing the tags to be applied to the resource. + - If the I(tags) parameter is not set then tags will not be modified. + type: dict + required: false + aliases: ['resource_tags'] + purge_tags: + description: + - If I(purge_tags=true) existing tags will be purged from the resource to + match exactly what is defined by I(tags) parameter. + - If the I(tags) parameter is not set then tags will not be modified. + - Tag keys beginning with C("aws:") are reserved by Amazon and can not be + modified. As such they will be ignored for the purposes of the + I(purge_tags) parameter. See the Amazon documentation for more information + U(https://docs.aws.amazon.com/general/latest/gr/aws_tagging.html#tag-conventions). + type: bool + default: true + required: false +''' + + # Minimum requirements for the collection + DEPRECATED_PURGE = r''' +options: + tags: + description: + - A dictionary representing the tags to be applied to the resource. + - If the I(tags) parameter is not set then tags will not be modified. + type: dict + required: false + aliases: ['resource_tags'] + purge_tags: + description: + - If I(purge_tags=true) existing tags will be purged from the resource to + match exactly what is defined by I(tags) parameter. + - If the I(tags) parameter is not set then tags will not be modified. + - Tag keys beginning with C("aws:") are reserved by Amazon and can not be + modified. As such they will be ignored for the purposes of the + I(purge_tags) parameter. See the Amazon documentation for more information + U(https://docs.aws.amazon.com/general/latest/gr/aws_tagging.html#tag-conventions). + - The current default value of C(False) has been deprecated. The default + value will change to C(True) in release 5.0.0. + type: bool + required: false +''' From d59c912094a4da9b59dc3fb44a773ff72b5252d3 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Tue, 31 May 2022 16:32:50 +0200 Subject: [PATCH 2/3] Apply suggestions from samccann Co-authored-by: Sandra McCann --- 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 5296dc6e9ab..38333eaad37 100644 --- a/docs/docsite/rst/dev_guidelines.rst +++ b/docs/docsite/rst/dev_guidelines.rst @@ -176,8 +176,7 @@ Module behavior --------------- To reduce the chance of breaking changes occurring when new features are added, -when a parameter is not explicitly set in a task, the module should avoid -modifying the resource attribute. +the module should avoid modifying the resource attribute when a parameter is not explicitly set in a task. By convention, when a parameter is explicitly set in a task, the module should set the resource attribute to match what was set in the task. In some cases, From faafa779bb563f1ede473f2ad2a40e9903a958d5 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Tue, 31 May 2022 17:30:31 +0200 Subject: [PATCH 3/3] More review comments --- docs/docsite/rst/dev_guidelines.rst | 11 +++++++---- plugins/doc_fragments/tags.py | 18 ++++++++++-------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/docs/docsite/rst/dev_guidelines.rst b/docs/docsite/rst/dev_guidelines.rst index 38333eaad37..b6cd9928c38 100644 --- a/docs/docsite/rst/dev_guidelines.rst +++ b/docs/docsite/rst/dev_guidelines.rst @@ -176,14 +176,17 @@ Module behavior --------------- To reduce the chance of breaking changes occurring when new features are added, -the module should avoid modifying the resource attribute when a parameter is not explicitly set in a task. +the module should avoid modifying the resource attribute when a parameter is +not explicitly set in a task. By convention, when a parameter is explicitly set in a task, the module should set the resource attribute to match what was set in the task. In some cases, such as tags or associations, it can be helpful to add an additional parameter -which changes this behavior to add the new setting to the attribute rather than -replacing the current attributes. Such a parameter should default to the -replacive rather than additive behavior. +which can be set to change the behavior from replacive to additive. However, the +default behavior should still be replacive rather than additive. + +See the `Dealing with tags` +section for an example with ``tags`` and ``purge_tags``. .. _ansible_collections.amazon.aws.docsite.dev_module_connection: diff --git a/plugins/doc_fragments/tags.py b/plugins/doc_fragments/tags.py index 869138445aa..cc2fa31eced 100644 --- a/plugins/doc_fragments/tags.py +++ b/plugins/doc_fragments/tags.py @@ -21,10 +21,11 @@ class ModuleDocFragment(object): aliases: ['resource_tags'] purge_tags: description: - - If I(purge_tags=true) existing tags will be purged from the resource to - match exactly what is defined by I(tags) parameter. - - If the I(tags) parameter is not set then tags will not be modified. - - Tag keys beginning with C("aws:") are reserved by Amazon and can not be + - If I(purge_tags=true) and I(tags) is set, existing tags will be purged + from the resource to match exactly what is defined by I(tags) parameter. + - If the I(tags) parameter is not set then tags will not be modified, even + if I(purge_tags=True). + - Tag keys beginning with C(aws:) are reserved by Amazon and can not be modified. As such they will be ignored for the purposes of the I(purge_tags) parameter. See the Amazon documentation for more information U(https://docs.aws.amazon.com/general/latest/gr/aws_tagging.html#tag-conventions). @@ -45,10 +46,11 @@ class ModuleDocFragment(object): aliases: ['resource_tags'] purge_tags: description: - - If I(purge_tags=true) existing tags will be purged from the resource to - match exactly what is defined by I(tags) parameter. - - If the I(tags) parameter is not set then tags will not be modified. - - Tag keys beginning with C("aws:") are reserved by Amazon and can not be + - If I(purge_tags=true) and I(tags) is set, existing tags will be purged + from the resource to match exactly what is defined by I(tags) parameter. + - If the I(tags) parameter is not set then tags will not be modified, even + if I(purge_tags=True). + - Tag keys beginning with C(aws:) are reserved by Amazon and can not be modified. As such they will be ignored for the purposes of the I(purge_tags) parameter. See the Amazon documentation for more information U(https://docs.aws.amazon.com/general/latest/gr/aws_tagging.html#tag-conventions).