Add ec2_asg_tag module #482

jsok · 2021-03-17T03:45:06Z

SUMMARY

A new module to manage ASG tags, analogous to the ec2_tag module in
amazon.aws collection.

Fixes #481

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

ec2_asg_tag

ADDITIONAL INFORMATION

Motivation is outlined in #481, but this module provides a more declarative approach to managing ASG tags, e.g.:

- name: Ensure tags are present on an ASG
  community.aws.ec2_asg_tag:
    resource: my-auto-scaling-group
    state: present
    tags:
      - environment: production
        propagate_at_launch: true
      - role: webserver
        propagate_at_launch: true

- name: Ensure tag is absent on an ASG
  community.aws.ec2_asg_tag:
    resource: my-auto-scaling-group
    state: absent
    tags:
      - environment: development

The module utilises underlying API calls to:

ansibullbot · 2021-03-17T03:51:37Z

cc @garethr @jillr @s-hertel @tremble @wimnat
click here for bot help

jsok · 2021-03-17T04:47:03Z

ansible/check failure seems to be unrelated to the change.

plugins/modules/ec2_asg_tag.py

markuman · 2021-03-18T18:30:40Z

recheck

markuman

LGTM
dunno if you get the 1.4.0 train. maybe it is to late.

plugins/modules/ec2_asg_tag.py

markuman

Oh, you need also a changelog fragment.

jsok · 2021-03-19T05:55:04Z

Oh, you need also a changelog fragment.

Can you please advise what a new module fragment looks like? I've been unable to find any such occurrence in the git log.

jsok · 2021-03-19T06:08:32Z

ready_for_review

A new module to manage ASG tags, analogous to the `ec2_tag` module in `amazon.aws` collection. Fixes ansible-collections#481

markuman · 2021-03-19T07:48:58Z

Oh, you need also a changelog fragment.

Can you please advise what a new module fragment looks like? I've been unable to find any such occurrence in the git log.

Take a look in the changelogs/fragments/ folder.
https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs
So you'll need a minor_changes fragment for a new module.

jsok · 2021-03-19T08:47:04Z

So you'll need a minor_changes fragment for a new module.

Reading that document suggests otherwise:

You do not have to add a changelog fragment for PRs that add new modules and plugins, because our tooling does that for you automatically.

jsok · 2021-03-19T08:54:34Z

recheck

jsok · 2021-03-19T08:56:03Z

/rebuild_failed

jsok · 2021-03-22T08:17:46Z

/rebuild

jsok · 2021-03-25T03:13:49Z

@markuman can I please get another review on this? The Shippable check if failing for unrelated reasons to this PR/change.

jillr

Thanks for this PR @jsok. We just recently migrated off of Shippable CI and onto Zuul (the ansible/check Checks) and it looks like this PR just got caught in that transition time, no need to worry about those failures.

jillr · 2021-04-19T22:28:28Z

plugins/modules/ec2_asg_tag.py

+    return tag_list
+
+
+def compare_asg_tags(current_tags_dict, new_tags_dict, purge_tags=True):


Could you please import and use the compare_aws_tags function from the module utility?
https://github.com/ansible-collections/amazon.aws/blob/main/plugins/module_utils/ec2.py#L783

@jillr this and the other requested changes weren't possible based on my testing.

ASG tags aren't just K/V pairs (as expected by compare_aws_tags, boto3_tag_list_to_ansible_dict and ansible_dict_to_boto3_tag_list), they're a list of dicts instead. The ramification here is that if propagate_at_launch is changed but the K/V doesn't, the module won't detect any tag changes which I believe is not the desired behaviour.

Another issue I ran into was that the to_text and to_native functions did some odd things, particularly around None values. Hence why compare_asg_tags doesn't use them like compare_aws_tags.
From memory you can't roundtrip None like to_native(to_text(None))... or maybe it was the other way around? 🤔

jillr · 2021-04-19T22:29:13Z

plugins/modules/ec2_asg_tag.py

+    return tags_to_set, tag_keys_to_unset
+
+
+def tag_list_to_dict(tag_list):


Same for boto3_tag_list_to_ansible_dict:
https://github.com/ansible-collections/amazon.aws/blob/main/plugins/module_utils/ec2.py#L465

jillr · 2021-04-19T22:30:32Z

plugins/modules/ec2_asg_tag.py

+from ansible_collections.amazon.aws.plugins.module_utils.ec2 import AWSRetry
+
+
+def to_boto3_tag_list(tags, group_name):


Could you please import and use the ansible_dict_to_boto3_tag_list function from the module utility?

jillr · 2021-04-19T22:34:11Z

tests/integration/targets/ec2_asg/tasks/main.yml

@@ -247,6 +247,124 @@
          - "output.tags | length == 1"
          - output is changed

+    # ============================================================
+


Could you please add some tests for adding and removing tags in check_mode?

jsok · 2021-04-19T23:05:40Z

Hi all, thanks for the reviews but unfortunately I won't have time to follow up on the requested changes (this PR was to help implement something at my previous role, however I've moved roles since raising the PR).

I'm going to close this PR but happy for someone else to pick up where I left off. Sorry for the inconvenience ❤️

ec2_asg: Add purge_tags to AutoScalingGroups. SUMMARY Add purge_tags to ec2_asg module. Fixes #481. ISSUE TYPE Feature Pull Request COMPONENT NAME ec2_asg ADDITIONAL INFORMATION There was another PR (currently closed) #482 - with similar functionality but I'm not sure if having modules to handle tags for individual services/modules is a good way to have this functionality. It will certainly cause increase in number of modules. Hence tried modifying existing ec2_asg module to be able to do this. This utilizes underlying API calls to: https://docs.aws.amazon.com/autoscaling/ec2/APIReference/API_DescribeTags.html https://docs.aws.amazon.com/autoscaling/ec2/APIReference/API_CreateOrUpdateTags.html https://docs.aws.amazon.com/autoscaling/ec2/APIReference/API_DeleteTags.html Reviewed-by: Alina Buzachis <None> Reviewed-by: Jill R <None> Reviewed-by: Mandar Kulkarni <[email protected]>

ec2_asg: Add purge_tags to AutoScalingGroups. SUMMARY Add purge_tags to ec2_asg module. Fixes ansible-collections#481. ISSUE TYPE Feature Pull Request COMPONENT NAME ec2_asg ADDITIONAL INFORMATION There was another PR (currently closed) ansible-collections#482 - with similar functionality but I'm not sure if having modules to handle tags for individual services/modules is a good way to have this functionality. It will certainly cause increase in number of modules. Hence tried modifying existing ec2_asg module to be able to do this. This utilizes underlying API calls to: https://docs.aws.amazon.com/autoscaling/ec2/APIReference/API_DescribeTags.html https://docs.aws.amazon.com/autoscaling/ec2/APIReference/API_CreateOrUpdateTags.html https://docs.aws.amazon.com/autoscaling/ec2/APIReference/API_DeleteTags.html Reviewed-by: Alina Buzachis <None> Reviewed-by: Jill R <None> Reviewed-by: Mandar Kulkarni <[email protected]>

…#483) ec2_vol: Fix incorrectly returned changed result SUMMARY When modify_volume is used but no new disk is being attached, the module incorrectly reports that no change has occurred even when disks have been modified (iops, throughput, type, etc.). This is due to the attach function overwriting the changed variable even if no disks are attached. Fixes ansible-collections#482 The integration test has been fixed so that when the gp3 modifications are tested, the volume is already in an attached state (previously detached) when reporting back changed. The detach tests are moved further down now, allowing this case to be properly covered by the existing assert: amazon.aws/tests/integration/targets/ec2_vol/tasks/tests.yml Lines 384 to 387 in e8df917 - name: check that volume_type has changed assert: that: - changed_gp3_volume.changed ISSUE TYPE Bugfix Pull Request COMPONENT NAME ec2_vol Reviewed-by: Alina Buzachis <None> Reviewed-by: None <None> Reviewed-by: Mark Chappell <None> Reviewed-by: None <None>

ansibullbot added community_review integration tests/integration module module needs_triage new_contributor Help guide this first time contributor new_module New module new_plugin New plugin plugins plugin (any type) tests tests labels Mar 17, 2021

jsok commented Mar 17, 2021

View reviewed changes

plugins/modules/ec2_asg_tag.py Outdated Show resolved Hide resolved

markuman approved these changes Mar 18, 2021

View reviewed changes

plugins/modules/ec2_asg_tag.py Show resolved Hide resolved

markuman requested changes Mar 18, 2021

View reviewed changes

ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed community_review labels Mar 18, 2021

jsok force-pushed the asg-tag branch 2 times, most recently from 9619414 to 03c7023 Compare March 19, 2021 06:04

ansibullbot added community_review and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Mar 19, 2021

Add ec2_asg_tag module

56657f6

A new module to manage ASG tags, analogous to the `ec2_tag` module in `amazon.aws` collection. Fixes ansible-collections#481

jsok force-pushed the asg-tag branch from 03c7023 to 56657f6 Compare March 19, 2021 06:20

ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed community_review labels Mar 19, 2021

jillr requested changes Apr 19, 2021

View reviewed changes

jsok closed this Apr 19, 2021

jsok deleted the asg-tag branch April 19, 2021 23:05

mandar242 mentioned this pull request Mar 2, 2022

ec2_asg: Add purge_tags to AutoScalingGroups. #960

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ec2_asg_tag module #482

Add ec2_asg_tag module #482

jsok commented Mar 17, 2021

ansibullbot commented Mar 17, 2021

jsok commented Mar 17, 2021

markuman commented Mar 18, 2021

markuman left a comment •

edited

Loading

markuman left a comment

jsok commented Mar 19, 2021 •

edited

Loading

jsok commented Mar 19, 2021

markuman commented Mar 19, 2021

jsok commented Mar 19, 2021

jsok commented Mar 19, 2021

jsok commented Mar 19, 2021

jsok commented Mar 22, 2021

jsok commented Mar 25, 2021

jillr left a comment

jillr Apr 19, 2021

jsok Apr 19, 2021

jillr Apr 19, 2021

jillr Apr 19, 2021

jillr Apr 19, 2021

jsok commented Apr 19, 2021

		return tag_list


		def compare_asg_tags(current_tags_dict, new_tags_dict, purge_tags=True):

		return tags_to_set, tag_keys_to_unset


		def tag_list_to_dict(tag_list):

		from ansible_collections.amazon.aws.plugins.module_utils.ec2 import AWSRetry


		def to_boto3_tag_list(tags, group_name):

Add ec2_asg_tag module #482

Add ec2_asg_tag module #482

Conversation

jsok commented Mar 17, 2021

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibullbot commented Mar 17, 2021

jsok commented Mar 17, 2021

markuman commented Mar 18, 2021

markuman left a comment • edited Loading

Choose a reason for hiding this comment

markuman left a comment

Choose a reason for hiding this comment

jsok commented Mar 19, 2021 • edited Loading

jsok commented Mar 19, 2021

markuman commented Mar 19, 2021

jsok commented Mar 19, 2021

jsok commented Mar 19, 2021

jsok commented Mar 19, 2021

jsok commented Mar 22, 2021

jsok commented Mar 25, 2021

jillr left a comment

Choose a reason for hiding this comment

jillr Apr 19, 2021

Choose a reason for hiding this comment

jsok Apr 19, 2021

Choose a reason for hiding this comment

jillr Apr 19, 2021

Choose a reason for hiding this comment

jillr Apr 19, 2021

Choose a reason for hiding this comment

jillr Apr 19, 2021

Choose a reason for hiding this comment

jsok commented Apr 19, 2021

markuman left a comment •

edited

Loading

jsok commented Mar 19, 2021 •

edited

Loading