Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ec2_vol: returns an up to date tag dict of the volume #241

Conversation

goneri
Copy link
Member

@goneri goneri commented Jan 15, 2021

This patch ensures a ec2_vol calls will return the up to date
tag structure.
Previously, the module was returning the origin tag dictionary.

@goneri goneri force-pushed the ec2_vol-returns-an-up-to-date-tag-dict-of-the-volume_10591 branch from 14741d2 to 30d8e6b Compare January 15, 2021 21:20
goneri added a commit to goneri/amazon.aws that referenced this pull request Jan 15, 2021
goneri added a commit to goneri/amazon.aws that referenced this pull request Jan 15, 2021
@ansibullbot
Copy link

@goneri: Greetings! Thanks for taking the time to open this pullrequest. In order for the community to handle your pullrequest effectively, we need a bit more information.

Here are the items we could not find in your description:

  • issue type

Please set the description of this pullrequest with this template:
https://raw.githubusercontent.com/ansible/ansible/devel/.github/PULL_REQUEST_TEMPLATE.md

click here for bot help

@ansibullbot
Copy link

@ansibullbot ansibullbot added has_issue integration tests/integration module module needs_info This issue requires further information. Please answer any outstanding questions needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly needs_triage plugins plugin (any type) tests tests labels Jan 15, 2021
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor niggles

@@ -77,6 +77,7 @@
- "'instance_id' in volume1.volume.attachment_set"
- not volume1.volume.attachment_set.instance_id
- not volume1.volume.encrypted
- volume1.volume.tags.ResourcePrefix == "{{ resource_prefix }}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's very easy to mangle the tags (see also #224 (review)) I'd like to see some slightly more comprehensive tests in here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tremble. Can you please elaborate on what you want?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make more sense to add the additional tests in #242 as that's where it looks like preserve_tags is being fixed, what do you think @tremble?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with the tag tests being added in either this PR or the next, I'd just like to ensure that they're added especially given we're messing with tagging. If you try to add them and the results get mangled I'm also good with that just being documented and them actually being added in another PR.

      tags:
        "lowercase spaced": 'hello cruel world'
        "Title Case": 'Hello Cruel World'
        CamelCase: 'SimpleCamelCase'
        snake_case: 'simple_snake_case'

This patch ensures a `ec2_vol` calls will return the up to date
tag structure.
Previously, the module was returning the origin tag dictionary.
@goneri goneri force-pushed the ec2_vol-returns-an-up-to-date-tag-dict-of-the-volume_10591 branch from 30d8e6b to fba1477 Compare January 18, 2021 15:42
goneri added a commit to goneri/amazon.aws that referenced this pull request Jan 18, 2021
@ansibullbot ansibullbot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jan 18, 2021
@jillr
Copy link
Collaborator

jillr commented Jan 18, 2021

As long as we add more tag tests in 242 and other reviewer's are OK this change lgtm

@goneri goneri merged commit bec6f91 into ansible-collections:main Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has_issue integration tests/integration module module needs_info This issue requires further information. Please answer any outstanding questions needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants