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.

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

rds_instance module and tests #43789

Merged
merged 31 commits into from
Aug 31, 2018
Merged

Conversation

s-hertel
Copy link
Contributor

@s-hertel s-hertel commented Aug 7, 2018

SUMMARY

New boto3 rds_instance module that supports encryption and aurora with integration tests.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

rds_instance

ANSIBLE VERSION
ansible 2.7.0.dev0
ADDITIONAL INFORMATION

Fixes #19524
Fixes #11159
Fixes #20395
Fixes #22617
Fixes #24415
Fixes #29831
Fixes #24268
Fixes #28981
Fixes #34067
Fixes #38073
Fixes #30020
Fixes #30094
Fixes #34064
Fixes #23618

@ansibot
Copy link
Contributor

ansibot commented Aug 7, 2018

Hi @s-hertel,

Thank you for the pullrequest, just so you are aware we have a dedicated Working Group for aws.
You can find other people interested in this in #ansible-aws on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@ansibot ansibot added affects_2.7 This issue/PR affects Ansible v2.7 aws cloud core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. labels Aug 7, 2018
@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Aug 7, 2018
@dmsimard
Copy link
Contributor

dmsimard commented Aug 7, 2018

This is very timely -- thanks for you work !
It comes as I was looking at the existing rds module and noticing it came short on a couple points.

I'll test it out, review and report back.

@s-hertel s-hertel force-pushed the rds_instance_module branch 4 times, most recently from ebc9efe to 85048a7 Compare August 7, 2018 17:55
@ansibot
Copy link
Contributor

ansibot commented Aug 7, 2018

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/modules/cloud/amazon/rds_instance.py:760:12: duplicate-except Catching previously caught exception type ClientError
lib/ansible/modules/cloud/amazon/rds_instance.py:956:16: duplicate-except Catching previously caught exception type ClientError

The test ansible-test sanity --test integration-aliases [explain] failed with 1 error:

test/integration/targets/rds_instance/aliases:0:0: conflicting alias `shippable/aws/group[1-2]` and `unsupported`

The test ansible-test sanity --test pep8 [explain] failed with 3 errors:

lib/ansible/module_utils/aws/core.py:292:1: E302 expected 2 blank lines, found 1
lib/ansible/module_utils/aws/core.py:300:1: E302 expected 2 blank lines, found 1
lib/ansible/module_utils/aws/waiters.py:315:8: E121 continuation line under-indented for hanging indent

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Aug 7, 2018
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Aug 7, 2018
hyphens and the first character must be a letter and may not end in a hyphen or contain consecutive hyphens.
aliases:
- cluster_id
db_instance_class:
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't gone through all the options yet but I think it would be great if we could make ec2_instance and rds_instance as close as possible in terms of how they are used.
This might translate to changing something here or changing something in ec2_instance if we feel the implementation here is better.

For example, ec2_instance has instance_type for picking the flavor of the instance but here it is db_instance_class instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks so much for reviewing @dmsimard. Let me know if you notice anything else.

My implementation relies on the module options being convertible to the CamelCase boto3 method parameters, so I've been adding aliases rather than modifying the options. Does that work for you? I'll comb through the options for any in common in ec2_instance and add them as aliases. I think modifying the options themselves would make the module harder to maintain (note that this module has 75 options whereas ec2_instance has 35; I want to keep things as simple as possible while being user-friendly).

Copy link
Contributor

Choose a reason for hiding this comment

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

If AWS can't be bothered to make RDS and EC2 consistent, we shouldn't strive too hard to do so either :)

@ryansb
Copy link
Contributor

ryansb commented Aug 27, 2018

When making a long-running change, like instance size, if you make the same change successively like this:

---
- hosts: localhost
  module_defaults:
    rds_instance:
      profile: slscode
      region: us-east-2
    group/aws:
      profile: slscode
      region: us-east-2
  tasks:
  - rds_subnet_group:
      name: test-sub-group
      state: present
      description: fake-group
      subnets:
        - subnet-08370cc6c83f0d865
        - subnet-0761385c754f09bb7
  - rds_instance:
      id: rsb-test
      db_name: rsbio
      engine: postgres
      instance_type: db.t2.micro
      db_subnet_group_name: test-sub-group
      password: 123456789
      username: rsb
      allocated_storage: 10
      multi_az: true
      apply_immediately: true
      wait: true
      vpc_security_group_ids:
        - sg-00a8629192b8df910

  - pause: minutes=3

  - rds_instance:
      id: rsb-test
      db_name: rsbio
      engine: postgres
      instance_type: db.t2.small
      db_subnet_group_name: test-sub-group
      password: 123456789
      username: rsb
      allocated_storage: 10
      multi_az: true
      apply_immediately: true
      wait: true
      vpc_security_group_ids:
        - sg-00a8629192b8df910

  - rds_instance:
      id: rsb-test
      db_name: rsbio
      engine: postgres
      instance_type: db.t2.small
      db_subnet_group_name: test-sub-group
      password: 123456789
      username: rsb
      allocated_storage: 10
      multi_az: true
      apply_immediately: true
      wait: true
      vpc_security_group_ids:
        - sg-00a8629192b8df910

The second task with db.t2.small instance size will fail because the modification from the first task was apply_immediately: true and so the instance is in the modifying state, causing the module to fail.

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Aug 28, 2018
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Aug 29, 2018
@ansibot
Copy link
Contributor

ansibot commented Aug 30, 2018

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/rds_instance.py:733:161: E501 line too long (180 > 160 characters)

click here for bot help

)

if not module.boto3_at_least('1.5.0'):
module.fail_json(msg="rds_intance requires boto3 > 1.5.0")
Copy link
Contributor

Choose a reason for hiding this comment

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

minor typo

@s-hertel
Copy link
Contributor Author

bot_status

@willthames willthames merged commit 113336d into ansible:devel Aug 31, 2018
@willthames
Copy link
Contributor

Merged! Woohoo! At long last! Well done @s-hertel!

@s-hertel
Copy link
Contributor Author

Thanks @willthames @ryansb @dmsimard for helping with this! I believe all comments have been addressed, so please open bug reports if you find anything new.

@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.7 This issue/PR affects Ansible v2.7 aws cloud feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
5 participants