-
Notifications
You must be signed in to change notification settings - Fork 398
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: Add feature to create multiple health checks without updating existing health check #1143
route53_health_check: Add feature to create multiple health checks without updating existing health check #1143
Conversation
some documentation mismatches
|
fixed. |
This comment was marked as outdated.
This comment was marked as outdated.
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.
My first impression is that the bug is inside find_health_check()
. This function doesn't consider the path while filtering and, looking at this task example, since all of the other parameters match, it will update the health check instead of creating a new one (because it will match the health check even if the path is different).
I would probably add the path as a matching criteria for find_health_check()
and see what happens.
Probably we don't need to add a new parameter.
Hi @alinabuzachis, adding There is a possiblity of having And if we try to update that So modified a code block inside But this change again
In my opinion, |
This comment was marked as outdated.
This comment was marked as outdated.
My suggestion should create a different heath checks every time the existing resource_path is different than the new one we want to set. The problem may arise when an update should be performed. One solution would be to only update resource_path when None is defined, otherwise create a new one always. If you have already a resource_path value and you want to update it, you should first assign None and then update with the new value (Similar to how that should work for IPAddress https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/route53.html#Route53.Client.update_health_check. Not sure if I really like the approach). The other solution that comes to mind (and as Mandar suggested) is adding an additional param. |
If something like that will be introduces, it must be But the entire problem is spread over multiple collections and modules ( One major issue is, that there are more options per health check that are currently not suppored by community.aws, when I just take a look at the console. the entire strictly speaking, the only way to update an existing route53 health check is, when you provide its id (because AWS doesn't provide a primary key defined by the user). otherwhise the module must always create a new health check. So the current behaviour must be treated as a bug.
That also means, that a |
With the In my opinion I think the way suggested by @markuman is best way possible here.
My only concern here though is that could this change break any existing playbooks that are used for updating health checks? |
If the id is not provided, keep the behaviour as it is, just throw warnings that it will change in the future... |
@markuman I guess we can use directly community.aws.route53_info to query health checks. We have the same issue in community.aws.route53_info regarding the returned result, as already noted in ecs_taskdefinition. The output is not returned as snake_case but as CamelCase. To unsure consistency across all the modules, we should have snake_case results in my opinion. We could probably deprecate the CamelCase result and introduce a new result dict containing the snake_case params/ or directly the snake_case params inside the existing result. How would you suggest to handle that from community perspective? |
I fully agree here. |
That works also for me. But the |
some other thoughts I came across with .... |
@markuman Like this approach! |
@markuman @alinabuzachis I like the approach here. But would this need to add |
Ups, wrong clicked ... sry |
changelogs/fragments/1143-route53_health_check-update-delete-by-id-and-name.yml
Outdated
Show resolved
Hide resolved
Pull request merge failed: Resource not accessible by integration, You may need to manually rebase your PR and retry. |
regate |
Pull request merge failed: Resource not accessible by integration, You may need to manually rebase your PR and retry. |
Backport to stable-4: 💚 backport PR created✅ Backport PR branch: Backported as #1324 🤖 @patchback |
…thout updating existing health check (#1143) * Add health_check_name / name parameter support to naming health checks during creation * Handle create/update when health_check when name is unique identifier * Fix bug: tags not specified but name is specified resulting in python error * Add integration tests for as unique identifier * code cleanup to remove redundant usage of manage_tags * Feat: ability to Update and Delete health checks by ID * Add integration tests for testing update/delete health check by ID * Minor fix, improve login to handle find_health_check() call * Use required_together param * Added changelogs fragment * use fqdn for examples, fix changelog variable names (cherry picked from commit 8d3fec8)
…thout updating existing health check (#1143) (#1324) [PR #1143/8d3fec8e backport][stable-4] route53_health_check: Add feature to create multiple health checks without updating existing health check This is a backport of PR #1143 as merged into main (8d3fec8). SUMMARY Might fix #88 Update [06/30/2022]: This PR adds three new parameters to route53_health_check module. health_check_id=dict(type='str', aliases=['id'], required=False), health_check_name=dict(type='str', aliases=['name'], required=False), use_unique_names=dict(type='bool', required=False), health_check_id (alias id): when set, can be used to update and also delete health checks. use_unique_names and health_check_name (alias name): these are used together to make use of health check name as a unique identifier to create/update/delete health check and also assign a name to the health check. Update [05/20/2022]: Based on discussion between 15 May to 19 May below, the approach of adding ignore_existing parameter has been discarded for the time being. The progress on this WIP PR put on hold until route53_info return value issue gets resolved. #1236 Added parameter ignore_existing that allows to create multiple route53 healthchecks with similar parameters instead of updating existing healthcheck (example: create multiple route53 healthchecks with different paths to same server). ISSUE TYPE Feature Pull Request COMPONENT NAME route53_health_check ADDITIONAL INFORMATION Consider following example from the issue. - name: set up route53 health checks connection: local become: false route53_health_check: failure_threshold: 3 ip_address: '{{ ip_address }}' port: 80 request_interval: 30 resource_path: '{{ item }}' state: present type: HTTP #ignore_existing: True register: result with_items: - /some_status1 - /other_status2 With ignore_exisitng parameter, it will create two healthchecks to same sever with different resource_path. Without ignore_existing parameter, it will create first healthcheck to sever, then start creating the second healthcheck but as it's having same server/ip_address, it updates the existing healthcheck instead, so creates only single healthcheck at the end of the task. Reviewed-by: Mark Chappell <None>
…thout updating existing health check (ansible-collections#1143) * Add health_check_name / name parameter support to naming health checks during creation * Handle create/update when health_check when name is unique identifier * Fix bug: tags not specified but name is specified resulting in python error * Add integration tests for as unique identifier * code cleanup to remove redundant usage of manage_tags * Feat: ability to Update and Delete health checks by ID * Add integration tests for testing update/delete health check by ID * Minor fix, improve login to handle find_health_check() call * Use required_together param * Added changelogs fragment * use fqdn for examples, fix changelog variable names This commit was initially merged in https://github.com/ansible-collections/community.aws See: ansible-collections@8d3fec8
Merge release changelogs into main branch SUMMARY Cherrypicks from: Prepare 3.5.0 release (ansible-collections#1124) Prepare release 4.3.0 (#1125) Prepare 5.0.1 with minor bugfixes (ansible-collections#1123) Prepare 5.0.2 release (ansible-collections#1143) ISSUE TYPE Docs Pull Request COMPONENT NAME Merge in the changelogs from 3.5.0, 4.3.0, 5.0.1 and 5.0.2 into the main changelog ADDITIONAL INFORMATION Reviewed-by: Gonéri Le Bouder <goneri@lebouder.net>
SUMMARY
Might fix #88
Update [06/30/2022]:
This PR adds three new parameters to
route53_health_check
module.health_check_id
(aliasid
): when set, can be used toupdate
and alsodelete
health checks.use_unique_names
andhealth_check_name
(aliasname
): these are used together to make use of health check name as a unique identifier to create/update/delete health check and also assign a name to the health check.Update [05/20/2022]:
Based on discussion between 15 May to 19 May below, the approach of adding
ignore_existing
parameter has been discarded for the time being.The progress on this WIP PR put on hold until route53_info return value issue gets resolved. #1236
Added parameter
ignore_existing
that allows to create multiple route53 healthchecks with similar parameters instead of updating existing healthcheck (example: create multiple route53 healthchecks with different paths to same server).ISSUE TYPE
COMPONENT NAME
route53_health_check
ADDITIONAL INFORMATION
Consider following example from the issue.
With
ignore_exisitng
parameter, it will create two healthchecks to same sever with differentresource_path
.Without
ignore_existing
parameter, it will create first healthcheck to sever, then start creating the second healthcheck but as it's having same server/ip_address, it updates the existing healthcheck instead, so creates only single healthcheck at the end of the task.