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.

Sign up for GitHub

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

Jump to bottom

ec2_vol: preserve the name tag of the volume #242

Merged
merged 8 commits into from
Mar 12, 2021
Merged

ec2_vol: preserve the name tag of the volume #242

merged 8 commits into from
Mar 12, 2021

Conversation

goneri
Copy link
Member

@goneri goneri commented Jan 15, 2021 •
edited
Loading

Closes: #229
Closes: #276

@goneri goneri force-pushed the ec2_vol-preset-the-name-tag-of-the-volume_2707 branch from 4b61d98 to e525866 Compare January 15, 2021 21:42
@goneri goneri mentioned this pull request Jan 15, 2021
Closed
@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

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

@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
@goneri goneri force-pushed the ec2_vol-preset-the-name-tag-of-the-volume_2707 branch 3 times, most recently from 559c5cd to 3b7c6e8 Compare January 18, 2021 15:46
@ansibullbot ansibullbot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jan 18, 2021
@goneri
Copy link
Member Author

goneri commented Jan 18, 2021

note: Tests need #241 to pass

@jillr jillr mentioned this pull request Jan 18, 2021
Merged
@goneri goneri force-pushed the ec2_vol-preset-the-name-tag-of-the-volume_2707 branch from 3b7c6e8 to 48d8424 Compare January 18, 2021 18:54
@goneri goneri force-pushed the ec2_vol-preset-the-name-tag-of-the-volume_2707 branch from 48d8424 to 4e4cba7 Compare January 18, 2021 20:12
@goneri goneri self-assigned this Jan 20, 2021
@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Jan 20, 2021
@goneri goneri requested review from tremble and jillr January 20, 2021 19:58
jillr
jillr approved these changes Jan 20, 2021
View reviewed changes
Copy link
Collaborator

@jillr jillr left a comment

Choose a reason for hiding this comment

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

Thanks @goneri!

tremble
tremble requested changes Jan 21, 2021
View reviewed changes
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.

We specifically want to know that we're not breaking the Name tag or removing tags that we didn't try to set.

I have a preference that we'd expose a "purge_tags" option rather than hard coding either. But I'm good with that being a separate PR (that I'd be willing to raise)

tests/integration/targets/ec2_vol/tasks/tests.yml Show resolved Hide resolved
tests/integration/targets/ec2_vol/tasks/tests.yml Outdated Show resolved Hide resolved
tests/integration/targets/ec2_vol/tasks/tests.yml Show resolved Hide resolved
tests/integration/targets/ec2_vol/tasks/tests.yml Show resolved Hide resolved
@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Jan 21, 2021
@goneri
Copy link
Member Author

goneri commented Jan 21, 2021

We specifically want to know that we're not breaking the Name tag or removing tags that we didn't try to set.

I have a preference that we'd expose a "purge_tags" option rather than hard coding either. But I'm good with that being a separate PR (that I'd be willing to raise)

I would prefer to ask the user to use ec2_tag instead.

@tremble
Copy link
Contributor

tremble commented Jan 21, 2021

https://docs.ansible.com/ansible/latest/dev_guide/platforms/aws_guidelines.html#dealing-with-tags

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.

I think we should chose something and be consistent, updating the doc above to reflect this.

The advantage of having the module manage tagging shows up during creation of resources, since it lets you identify them from creation. If we're letting people manage tags on creation it makes sense to have a consistent interface that lets them manage them during the whole lifecycle.

@goneri goneri changed the title ec2_vol: preset the name tag of the volume ec2_vol: preserve the name tag of the volume Jan 22, 2021
@goneri goneri force-pushed the ec2_vol-preset-the-name-tag-of-the-volume_2707 branch from 78d301b to 98976ab Compare February 1, 2021 21:52
jillr
jillr reviewed Feb 19, 2021
View reviewed changes
plugins/modules/ec2_vol.py Outdated Show resolved Hide resolved
@jillr jillr requested a review from tremble February 19, 2021 17:18
tremble
tremble requested changes Feb 19, 2021
View reviewed changes
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.

Apologies if I'm failing to spot it, I don't see an integration test for purge_tags: True

plugins/modules/ec2_vol.py Outdated Show resolved Hide resolved
tests/integration/targets/ec2_vol/tasks/tests.yml Show resolved Hide resolved
@gravesm gravesm mentioned this pull request Mar 5, 2021
Closed
tremble
tremble requested changes Mar 11, 2021
View reviewed changes
changelogs/fragments/241_ec2_vol-add_purge_tags_parameter.yaml Outdated Show resolved Hide resolved
plugins/modules/ec2_vol.py Outdated Show resolved Hide resolved
@tremble tremble merged commit 6d0a274 into ansible-collections:main Mar 12, 2021
@tremble tremble mentioned this pull request Apr 13, 2021
Closed
@jillr jillr mentioned this pull request May 3, 2021
Closed
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
@tamirhad
ECS service - add tag+propagate_tags upon creation. (ansible-collecti…
c7c8090 
…ons#543)

ECS service - add tag+propagate_tags upon creation.

SUMMARY

this PR is continuation of ansible-collections#242
this PR will enable the use of tags at creation time, in addition to propagate_tags.

ISSUE TYPE


Feature Pull Request

COMPONENT NAME

ecs_service
ADDITIONAL INFORMATION


this PR is continuation of ansible-collections#242
i cant contribute to his repo as i am not a maintainer so i created a new PR, if its not the right git flow please be gentle, its my first time :)

Reviewed-by: Mark Chappell <None>
Reviewed-by: None <None>
Reviewed-by: Alina Buzachis <None>
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
@tamirhad @abikouo
ECS service - add tag+propagate_tags upon creation. (ansible-collecti…
4c75100 
…ons#543)

ECS service - add tag+propagate_tags upon creation.

SUMMARY

this PR is continuation of ansible-collections#242
this PR will enable the use of tags at creation time, in addition to propagate_tags.

ISSUE TYPE


Feature Pull Request

COMPONENT NAME

ecs_service
ADDITIONAL INFORMATION


this PR is continuation of ansible-collections#242
i cant contribute to his repo as i am not a maintainer so i created a new PR, if its not the right git flow please be gentle, its my first time :)

Reviewed-by: Mark Chappell <None>
Reviewed-by: None <None>
Reviewed-by: Alina Buzachis <None>
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Oct 24, 2023
@tamirhad
ECS service - add tag+propagate_tags upon creation. (ansible-collecti…
e3c8302 
…ons#543)

ECS service - add tag+propagate_tags upon creation.

SUMMARY

this PR is continuation of ansible-collections#242
this PR will enable the use of tags at creation time, in addition to propagate_tags.

ISSUE TYPE


Feature Pull Request

COMPONENT NAME

ecs_service
ADDITIONAL INFORMATION


this PR is continuation of ansible-collections#242
i cant contribute to his repo as i am not a maintainer so i created a new PR, if its not the right git flow please be gentle, its my first time :)

Reviewed-by: Mark Chappell <None>
Reviewed-by: None <None>
Reviewed-by: Alina Buzachis <None>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jillr jillr jillr approved these changes

@tremble tremble tremble approved these changes

Assignees

@goneri goneri

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
Milestone
No milestone
4 participants
@goneri @ansibullbot @tremble @jillr