-
Notifications
You must be signed in to change notification settings - Fork 336
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
route53_health_check: Fix "Name" tag key removal idempotentcy issue #1253
route53_health_check: Fix "Name" tag key removal idempotentcy issue #1253
Conversation
# Add 'Name' tag to add name to health check | ||
if not tags: | ||
tags = {} | ||
tags['Name'] = health_check_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do this, you need to make sure "purge_tags" is updated to False, if you don't, then leaving tags
empty results in all non-Name tags being purged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I follow,
if user does not change anything in the task, run it multiple times, with tags being read into the module from the task here, it shouldn't cause issue and should not remove non-name tags
if the user runs the task, then on the second run removes the tags from the task (keeping everything else same), then definitely module will remove the changed tags (still keeping Name
tag), I assume that's the behavior we currently have and are aiming for right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for current code change purge_tags=False
does not remove existing tags.
Added integration test to verify the same
https://github.com/ansible-collections/amazon.aws/pull/1253/files#diff-0d5db09c227a69a9880f5e7f16fde22576839c70c836a2836edbee1cee26989dR114-R146
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what @tremble is saying (feel free to correct me) is that because purge_tags
by default is true
, if the user has not set tags
, what you are doing here is likely different than what they intend. With this logic, what will effectively happen is the following, even if they had not set tags
:
route53_health_check:
...
tags:
Name: whatever
purge_tags: true
This means the only tag remaining after the task runs will be the Name
tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we would want from a testing scenario is to have the task that sets use_unique_names
, sets purge_tags=true
(or just doesn't set it, since this is the default), and does not set the tags
parameter. Then check to make sure that this does not remove the other non-Name
tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, added integration tests as well.
Now code behavior is as below
- tags={}, purge_tags=false do nothing
- tags={}, purge_tags=true remove existing tags except 'Name' tag
- tags=None, purge_tags=true do nothing
tests/integration/targets/route53_health_check/tasks/create_delete_by_name.yml
Outdated
Show resolved
Hide resolved
Moving PR to WIP, adding more tests for asserting that tags are added/removed as per expected behavior. |
recheck |
2 similar comments
recheck |
recheck |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sanity test failing for unrelated PR's changelog
|
This comment was marked as outdated.
This comment was marked as outdated.
recheck |
This comment was marked as outdated.
This comment was marked as outdated.
d215048
to
e115368
Compare
Docs Build 📝Thank you for contribution!✨ This PR has been merged and your docs changes will be incorporated when they are next published. |
Prepare release 4.0.0 SUMMARY Prepare release 4.0.0 Changelogs galaxy.yml Updated docs ISSUE TYPE Docs Pull Request COMPONENT NAME CHANGELOG.rst README.md changelogs/changelog.yaml galaxy.yml docs/ ADDITIONAL INFORMATION Reviewed-by: Markus Bergholz <[email protected]> Reviewed-by: Mandar Kulkarni <[email protected]> Reviewed-by: Alina Buzachis <None>
SUMMARY
Depends-On: #1280
Fixes #1188
When using
health_check_name
as unique identifier (settinguse_unique_names: True
and providing ahealth_check_name
) andhealth_check
tagsare set
,Current logic for adding name to a
health_check
causes an issue when rerunning the create/update task.While ideally it should be idempotent, it removes the 'Name' tag (used for health_check_name) causing removal of health check name.
ISSUE TYPE
COMPONENT NAME
route53_health_check
ADDITIONAL INFORMATION
To test, run the following sample playbook task twice
Expected output: Health check name should not disapper (i.e. 'Name' tag should not get removed on rerun)