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

Remove 'ResourceRecords' when 'AliasTarget' #502

Merged
merged 6 commits into from
Apr 5, 2021
Merged

Remove 'ResourceRecords' when 'AliasTarget' #502

merged 6 commits into from
Apr 5, 2021
+143 −62

Conversation

notatoad
Copy link
Contributor

@notatoad notatoad commented Mar 26, 2021 •
edited
Loading

SUMMARY

sending a change to the route53 api that includes both an AliasTarget and a ResourceRecord causes the api to return with an error. removing the ResourceRecord when an AliasTarget is present allows this module to successfully modify an alias record

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

route53

ADDITIONAL INFORMATION

calling the following

community.aws.route53:
    state: present
    zone: "{{ valid_zone_identifier }}"
    record: "{{ record_name }}"
    type: A
    alias: true
    alias_hosted_zone_id: "{{ valid_zone_id }}"
    ttl: null
    value: "{{ application_loadbalancer_domain_name }}"
    overwrite: true

results in the following error:

'Failed to update records: An error occurred (InvalidInput) when calling the ChangeResourceRecordSets operation: Invalid request: Expected exactly one of [AliasTarget, all of [TTL, and ResourceRecords], or TrafficPolicyInstanceId], but found more than one in Change with [Action=UPSERT, Name=example.com., Type=A, SetIdentifier=null]'

the attached change removes the 'ResourceRecord' from the ChangeBatch, and the record is created without error

@ansibullbot
Copy link

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

@ansibullbot ansibullbot added bug This issue/PR relates to a bug community_review module module needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) small_patch Hopefully easy to review labels Mar 26, 2021
@notatoad
Copy link
Contributor Author

i think this fix resolves #434 (sorry, didn't look closely enough at bug reports before making a PR)

my familiarity with this code is minimal, but it works on my machine :)
boto3_version: 1.17.38
botocore_version: 1.20.38

@ansibullbot 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 26, 2021
tremble
tremble requested changes Mar 27, 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.

Thanks for taking the time to open this PR. A couple of things

  1. Sanity tests are failing due to extra spaces in the 'empty' line you added.
  2. Please add a changelog: https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs-how-to
  3. To reduce the chance of a regression, please a test to our integration tests: tests/integration/targets/route53/tasks/

plugins/modules/route53.py Outdated Show resolved Hide resolved
@boutetnico
Copy link
Contributor

@notatoad I have tested your PR and it does not fix #434 on my end. However @Surgo patch does fix the issue.

boto3==1.17.39
botocore==1.20.39

@ansibullbot 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 Apr 5, 2021
notatoad and others added 5 commits April 5, 2021 16:08
@notatoad @tremble
Remove 'ResourceRecords' when 'AliasTarget'
a729b5f 
sending a change to the route53 api that includes both an AliasTarget and a ResourceRecord causes the api to return with an error.  removing the ResourceRecord when an AliasTarget is preset allows this module to continue without error
@tremble
Cleanup tests and use RFC2602 Domains and RFC5737 CIDRs
1fade08
@tremble
Add integration test for aliases
6e9808a
@tremble
Make Alias and TTL mutually exclusive
851de17
@tremble
Update docs to list region/failover/weight as mutually exclusive.
3f0c956
@ansibullbot ansibullbot added integration tests/integration tests tests and removed small_patch Hopefully easy to review labels Apr 5, 2021
@tremble
Copy link
Contributor

tremble commented Apr 5, 2021

@boutetnico @Surgo I've tweaked the PR and added some tests. Could you try out the latest version from this PR?

@boutetnico
Copy link
Contributor

@tremble I've tested your changes. They allowed me to spot a mistake in my playbook, I was using the ttl option with alias: true. LGTM 👍🏻 .

@jillr
Copy link
Collaborator

jillr commented Apr 5, 2021

recheck

@tremble
Copy link
Contributor

tremble commented Apr 5, 2021

Approved with all checks passing.

@tremble tremble merged commit 8a2a138 into ansible-collections:main Apr 5, 2021
@tremble
Copy link
Contributor

tremble commented Apr 5, 2021

@notatoad Thanks for your original submission
@boutetnico and @Surgo Many thanks for testing

This was referenced Apr 8, 2021
Closed
Closed
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
@notatoad
Remove 'ResourceRecords' when 'AliasTarget' (ansible-collections#502)
b08cf56 
* Remove 'ResourceRecords' when 'AliasTarget'

sending a change to the route53 api that includes both an AliasTarget and a ResourceRecord causes the api to return with an error.  removing the ResourceRecord when an AliasTarget is preset allows this module to continue without error

* Cleanup tests and use RFC2602 Domains and RFC5737 CIDRs
* Add integration test for aliases
* Make Alias and TTL mutually exclusive
* Update docs to list region/failover/weight as mutually exclusive.
* changelog
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
@notatoad
Remove 'ResourceRecords' when 'AliasTarget' (ansible-collections#502)
51bd728 
* Remove 'ResourceRecords' when 'AliasTarget'

sending a change to the route53 api that includes both an AliasTarget and a ResourceRecord causes the api to return with an error.  removing the ResourceRecord when an AliasTarget is preset allows this module to continue without error

* Cleanup tests and use RFC2602 Domains and RFC5737 CIDRs
* Add integration test for aliases
* Make Alias and TTL mutually exclusive
* Update docs to list region/failover/weight as mutually exclusive.
* changelog
@tremble tremble mentioned this pull request Sep 27, 2021
Closed
danielcotton pushed a commit to danielcotton/community.aws that referenced this pull request Nov 23, 2021
@notatoad @danielcotton
Remove 'ResourceRecords' when 'AliasTarget' (ansible-collections#502)
c65b6cf 
* Remove 'ResourceRecords' when 'AliasTarget'

sending a change to the route53 api that includes both an AliasTarget and a ResourceRecord causes the api to return with an error.  removing the ResourceRecord when an AliasTarget is preset allows this module to continue without error

* Cleanup tests and use RFC2602 Domains and RFC5737 CIDRs
* Add integration test for aliases
* Make Alias and TTL mutually exclusive
* Update docs to list region/failover/weight as mutually exclusive.
* changelog
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request May 25, 2022
@tremble
Bump botocore to 1.18.0 prior to 2.0.0 (ansible-collections#502)
c2994b2
abikouo pushed a commit to abikouo/community.aws that referenced this pull request Oct 24, 2023
@notatoad @goneri
Remove 'ResourceRecords' when 'AliasTarget' (ansible-collections#502)
bb2dee7 
* Remove 'ResourceRecords' when 'AliasTarget'

sending a change to the route53 api that includes both an AliasTarget and a ResourceRecord causes the api to return with an error.  removing the ResourceRecord when an AliasTarget is preset allows this module to continue without error

* Cleanup tests and use RFC2602 Domains and RFC5737 CIDRs
* Add integration test for aliases
* Make Alias and TTL mutually exclusive
* Update docs to list region/failover/weight as mutually exclusive.
* changelog

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections@8a2a138
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boutetnico boutetnico boutetnico left review comments

@Surgo Surgo Surgo left review comments

@jillr jillr jillr approved these changes

@tremble tremble tremble approved these changes

Assignees
No one assigned
Labels
bug This issue/PR relates to a bug community_review integration tests/integration module module new_contributor Help guide this first time contributor plugins plugin (any type) tests tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@notatoad @ansibullbot @boutetnico @tremble @jillr @Surgo