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

Improve route53_facts to modern Ansible standards #31860

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
6 participants
@willthames
Contributor

willthames commented Oct 18, 2017

SUMMARY
  • Use built in retries, pagination
  • Return snake case
  • Document returns
  • Better tag handling
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

route53_facts

ANSIBLE VERSION
ansible 2.5.0 (devel 7623c2fbda) last updated 2017/10/18 12:38:39 (GMT +1000)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/will/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/will/src/ansible/lib/ansible
  executable location = /home/will/src/ansible/bin/ansible
  python version = 2.7.13 (default, Sep  5 2017, 08:53:59) [GCC 7.1.1 20170622 (Red Hat 7.1.1-3)]

@willthames

This comment has been minimized.

Contributor

willthames commented Oct 18, 2017

There are a lot of changes here, but given the module's current return values are undocumented and the module is in preview status, I hope that the changes are acceptable.

I have tested them against my existing route53 resources, they seem to work

@ansibot

This comment has been minimized.

Contributor

ansibot commented Oct 18, 2017

@willthames

This comment has been minimized.

Contributor

willthames commented Oct 18, 2017

@mikedlr this PR contains a change to module_utils.aws.core to support the deprecate method.

@willthames willthames force-pushed the willthames:route53_facts branch Oct 18, 2017

@gundalow gundalow removed the needs_triage label Oct 18, 2017

@weisslj

This comment has been minimized.

Contributor

weisslj commented Oct 19, 2017

I have tested this with our current route53 setup, which works.

@ansibot ansibot added the stale_ci label Oct 27, 2017

try:
return list_record_sets_with_backoff(client, params)
except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:
module.fail_json_aws(e, msg="Couldn't list record sets")

This comment has been minimized.

@s-hertel

s-hertel Oct 31, 2017

Contributor

If providing a hosted_zone_id that has been removed, this fails. Should it fail, or should record sets just be an empty dict if e.response['Error']['Code'] == 'NoSuchHostedZone' in list_record_sets_with_backoff()? Wondered something similar throughout reading this PR in a number of other places too.

This comment has been minimized.

@willthames

willthames Nov 1, 2017

Contributor

Thanks for this spot @s-hertel - it's a great catch. To be honest I think it's a flaw with AWSRetry that this happens - retries for not found should be handled by calling functions.

But until there's consensus on that, I'll add a fix for this.

Edit: Ah, I'm thinking of retries for NotFound codes, this is a different situation. It's a tough call - I'm not sure whether it's better to return an empty result for a non existent hosted zone - in some ways the error message is clearer.

lib/ansible/modules/cloud/amazon/route53_facts.py Outdated
results = client.list_reusable_delegation_sets(**params)
else:
if module.params.get('delegation_set_id'):
params['DelegationSetId'] = module.params.get('delegation_set_id')

This comment has been minimized.

@s-hertel

s-hertel Oct 31, 2017

Contributor

I think 'DelegationSetId' needs to be 'Id'.

This comment has been minimized.

@willthames

willthames Nov 1, 2017

Contributor

Wow, I did not change this, how long has this bug been present! (Thanks again for spotting this)

next_marker: "{{ first_facts.NextMarker }}"
max_items: 1
when: "{{ 'NextMarker' in first_facts }}"
RETURN = '''

This comment has been minimized.

@s-hertel

s-hertel Oct 31, 2017

Contributor

Hurrah for RETURN docs :)

Improve route53_facts to modern Ansible standards
* Use built in retries, pagination
* Return snake case
* Document returns
* Better tag handling
* Deprecate resource_id parameter
* Deprecate tags query method
* Fail on multiple resource_ids

@willthames willthames force-pushed the willthames:route53_facts branch to 0635992 Nov 1, 2017

@willthames

This comment has been minimized.

Contributor

willthames commented Nov 1, 2017

@s-hertel I've fixed the delegation set id bug. I think a module failure for a non existent hosted zone seems reasonable.

@ansibot ansibot removed the stale_ci label Nov 1, 2017

@@ -43,15 +43,8 @@
required: false
max_items:
description:
- Maximum number of items to return for various get/list requests
required: false
next_marker:

This comment has been minimized.

@ryansb

ryansb Nov 7, 2017

Contributor

Why delete the docs for the next_marker? IMO we should entirely remove the max_items and next_marker settings and use pagination internally, there doesn't seem to be a good reason to expose these. Obviously needs to be supported still for the deprecation cycle, but it seems like unnecessary complexity.

- name: setup of example for using next_marker
route53_facts:
query: hosted_zone
max_items: 1

This comment has been minimized.

@ryansb

ryansb Nov 7, 2017

Contributor

I get de-emphasizing next marker, but we shouldn't remove the example entirely I don't think.

resource_id = module.params.get('resource_id')
if resource_id:
if len(resource_id) > 1:
module.fail_json(msg='Using multiple resource_ids is no longer supported. Use a loop')

This comment has been minimized.

@ryansb

ryansb Nov 7, 2017

Contributor

I don't think we can fail out on this without a deprecation cycle first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment