New ansible module for aws Redshift and Redshift Subnet Group #185

Merged
merged 1 commit into from Sep 16, 2016

Projects

None yet
@j-carl
Contributor
j-carl commented Jan 6, 2015

Hey,

I coded two modules to handle AWS Redshift cluster and Redshift Subnet Group:

  • module redshift handles creating, deleting, modifying, and facts gathering of Redshift clusters
  • module redshift_subnet_group handles creating, modifying, and deleting of Redshift Subnet Groups

I used the modules rds and rds_subnet_group as templates to create these modules.

Jens

@gregdek
Contributor
gregdek commented Jul 4, 2015

@j-carl Thanks for submitting this new module.

To help facilitate reviews, we’ve broadened the number of people who can approve modules for inclusion into the Extras repository. The list of official reviewers can be found here:

https://github.com/ansible/ansible-modules-extras/blob/devel/REVIEWERS.md

Our new policy is that if a new module is reviewed and approved by at least two official module reviewers, and receives no -1 votes, the module will be approved for inclusion. We will be asking the community of reviewers to take a look at these modules on a regular basis.

To ensure that your module has the best chance of being approved, please double-check that you adhere to the Ansible module guidelines: http://docs.ansible.com/developing_modules.html#module-checklist

@robynbergeron
Contributor

Adding new process. We will be evaluating all new module PRs according to this process, effective immediately.

Thanks for submitting this new module to Ansible Extras! This module is now in community review, a process that is open to all Ansible users. In order for this module to be approved, it must gain the following votes:

“works_for_me”: If you have tested the module thoroughly, including testing of all of the module’s options, and if the module works for you, please add “works_for_me” in the comments.

“passes_guidelines”: If you have gone through the module guidelines and the module meets all of the requirements, please add “passes_guidelines” in the comments. Guidelines are available here: http://docs.ansible.com/developing_modules.html#module-checklist

“needs_revision”: If the module fails to work for you, or if it doesn’t meet guidelines, please add “needs_revision” in the comments with details about what needs to be fixed.

When a module has both “works_for_me” and “passes_guidelines” tags, we will promote the module for inclusion in Ansible Extras. At this point, you will be expected to maintain the module by fixing bugs and evaluating pull requests in a timely manner.

Thanks again for submitting your Ansible module!

@robynbergeron robynbergeron removed the P3 label Sep 25, 2015
@willthames willthames commented on an outdated diff Nov 3, 2015
cloud/amazon/redshift.py
+ redshift: authenticated redshift connection object
+
+ Returns:
+ """
+
+ identifier = module.params.get('identifier')
+ node_type = module.params.get('node_type')
+ username = module.params.get('username')
+ password = module.params.get('password')
+ wait = module.params.get('wait')
+ wait_timeout = module.params.get('wait_timeout')
+
+ changed = True
+ # Package up the optional parameters
+ params = {}
+ for p in ( 'db_name', 'cluster_type', 'cluster_security_groups', 'vpc_security_group_ids', 'cluster_subnet_group_name', 'availability_zone', 'preferred_maintenance_window', 'cluster_parameter_group_name', 'automated_snapshot_retention_period', 'port', 'cluster_version', 'allow_version_upgrade', 'number_of_nodes', 'publicly_accessible', 'encrypted', 'elastic_ip' ):
@willthames
willthames Nov 3, 2015 Contributor

I think this line could benefit from a bit of wrapping (arrays are pretty easy to split over lines after all)

@herbyg-axial

@j-carl , I added some improvements to this in #1365 - I'd like to know how best to proceed

@kylelundstedt

@robynbergeron Any word on whether the Redshift-related pull requests will be accepted soon? We'd love to see these features added to Ansible! Thanks!

@willthames willthames commented on an outdated diff Dec 18, 2015
cloud/amazon/redshift.py
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+DOCUMENTATION = '''
+---
+author: '"Jens Carl (@j-carl), Hothead Games Inc.'"
+module: redshift
+short_description: create, delete, or modify an Amazon redshift instance
+description:
+ - Creates, deletes, or modifies amazon Redshift cluster instances.
+options:
+ command:
+ description:
+ - Specifies the action to take.
+ required: true
+ default: null
@willthames willthames commented on an outdated diff Dec 18, 2015
cloud/amazon/redshift.py
+ choices: [ 'create', 'facts', 'delete', 'modify' ]
+ identifier:
+ description:
+ - Redshift cluster identifier.
+ required: true
+ default: null
+ node_type:
+ description:
+ - The node type of the cluster. Must be specified when command=create.
+ required: true
+ default: null
+ choices: ['dw1.xlarge', 'dw1.8xlarge', 'dw2.large', 'dw2.8xlarge', ]
+ username:
+ description:
+ - Master database username. Used only when command=create.
+ required: true
@willthames
willthames Dec 18, 2015 Contributor

If it's only used when command=create, it's not required - this does not match the module argument_spec

@willthames willthames commented on an outdated diff Dec 18, 2015
cloud/amazon/redshift.py
+options:
+ command:
+ description:
+ - Specifies the action to take.
+ required: true
+ default: null
+ choices: [ 'create', 'facts', 'delete', 'modify' ]
+ identifier:
+ description:
+ - Redshift cluster identifier.
+ required: true
+ default: null
+ node_type:
+ description:
+ - The node type of the cluster. Must be specified when command=create.
+ required: true
@willthames
willthames Dec 18, 2015 Contributor

This does not match the module argument_spec

@willthames willthames commented on an outdated diff Dec 18, 2015
cloud/amazon/redshift_subnet_group.py
+ - Cluster subnet group name.
+ required: true
+ aliases: ['name']
+ group_description:
+ description:
+ - Database subnet group description.
+ required: false
+ default: null
+ aliases: ['description']
+ group_subnets:
+ description:
+ - List of subnet IDs that make up the cluster subnet group.
+ required: false
+ default: null
+ aliases: ['subnets']
+ aws_secret_key:
@willthames
willthames Dec 18, 2015 Contributor

This is not needed - it will come from extends_documentation_fragment: aws

@willthames willthames commented on an outdated diff Dec 18, 2015
cloud/amazon/redshift_subnet_group.py
+ required: false
+ default: null
+ aliases: ['description']
+ group_subnets:
+ description:
+ - List of subnet IDs that make up the cluster subnet group.
+ required: false
+ default: null
+ aliases: ['subnets']
+ aws_secret_key:
+ description:
+ - AWS secret key. If not set then the value of the AWS_SECRET_KEY environment variable is used.
+ required: false
+ default: null
+ aliases: [ 'ec2_secret_key', 'secret_key' ]
+ aws_access_key:
@willthames
willthames Dec 18, 2015 Contributor

This is not needed - it will come from extends_documentation_fragment: aws

@willthames willthames commented on an outdated diff Dec 18, 2015
cloud/amazon/redshift.py
+ number_of_nodes = dict(type='int'),
+ publicly_accessible = dict(type='bool'),
+ encrypted = dict(type='bool'),
+ elastic_ip = dict(required=False),
+ new_cluster_identifier = dict(aliases=['new_identifier']),
+ wait = dict(type='bool', default=False),
+ wait_timeout = dict(default=300),
+ )
+ )
+
+ module = AnsibleModule(
+ argument_spec = argument_spec,
+ )
+
+ if not HAS_BOTO:
+ module.fail_json(msg='boto v2.9.0+ required for this module')
@willthames
willthames Dec 18, 2015 Contributor

I added a comment here about checking version, but from boto import redshift does that

@gregdek
Contributor
gregdek commented Mar 29, 2016

Thanks @j-carl for this PR. This PR requires revisions, either because it fails to build or by reviewer request. Please make the suggested revisions. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review.

[This message brought to you by your friendly Ansibull-bot.]

@j-carl
Contributor
j-carl commented Apr 6, 2016

Made some changes and it's ready_for_review.

@willthames willthames commented on an outdated diff Apr 7, 2016
cloud/amazon/redshift.py
+requirements: [ 'boto' ]
+extends_documentation_fragment: aws
+'''
+
+EXAMPLES = '''
+# Basic cluster provisioning example
+- redshift: >
+ command=create
+ node_type=dw1.xlarge
+ identifier=new_cluster
+ username=cluster_admin
+ password=1nsecure
+'''
+
+RETURN = '''
+changed:
@willthames
willthames Apr 7, 2016 Contributor

I assume changed isn't needed to be documented as it's a standard Ansible return value.

Thanks for adding the rest though

@willthames willthames commented on an outdated diff Apr 7, 2016
cloud/amazon/redshift_subnet_group.py
+ module: redshift_subnet_group
+ state: present
+ group_name: redshift-subnet
+ group_description: Redshift subnet
+ group_subnets:
+ - 'subnet-aaaaa'
+ - 'subnet-bbbbb'
+
+# Remove subnet group
+redshift_subnet_group: >
+ state: absent
+ group_name: redshift-subnet
+'''
+
+RETURN = '''
+changed:
@willthames
willthames Apr 7, 2016 Contributor

Is there no subnet group information returned? (as with the other module, I don't think changed needs to be documented). I'd say you're missing a trick if you're not returning anything (as things like subnet id are useful to be passed to later modules in the play)

@willthames willthames commented on an outdated diff Apr 7, 2016
cloud/amazon/redshift_subnet_group.py
+ module.fail_json(msg=str(e))
+
+ if state == 'absent':
+ if exists:
+ conn.delete_cluster_subnet_group(group_name)
+ changed = True
+
+ else:
+ if not exists:
+ new_group = conn.create_cluster_subnet_group(group_name, group_description, group_subnets)
+ else:
+ changed_group = conn.modify_cluster_subnet_group(group_name, group_subnets, description=group_description)
+ changed = True
+
+ except boto.exception.JSONResponseError, e:
+ # FIXME: Change this to just set the error message when
@willthames
willthames Apr 7, 2016 Contributor

delete the FIXMEs

@willthames
Contributor

Only really two remaining areas of concern - the amount of information that redshift_subnet_group returns could be vastly improved, and the FIXME comments should all go (as they won't be fixed in boto, so they'll never get fixed here). Once that's done this will meet guidelines. However, once this meets guidelines, I suspect we'll want to squash commits - so you might prefer to do that before the next revision round.

needs_revision

@willthames
Contributor

There is an argument for the facts stuff to be split into another module, and also an argument that this module should use state=present/absent. Given this module's provenance, (it's based on rds which has the same flaws), I'd just say we accept this once the remaining wrinkles are gone so that people can actually use redshift, and let the perfect implementation come later.

@gregdek
Contributor
gregdek commented Apr 7, 2016

Thanks @j-carl for this new module. When this module receives 'shipit' comments from two community members and any 'needs_revision' comments have been resolved, we will mark for inclusion.

[This message brought to you by your friendly Ansibull-bot.]

@j-carl
Contributor
j-carl commented Apr 8, 2016

@willthames I didn't add any information to the redshift_subnet_group module, because the underlying function doesn't expose any id or so. If you want to use the group in other Redshift things you the group-name, which you already specified in the module (parameter: name). But I can add it if you really want.

@j-carl
Contributor
j-carl commented Apr 11, 2016

@willthames On second thought I added some information for the module redshift_subnet_group and squashed my commits into one. So again it's ready_for_review.

@linuxdynasty
Contributor

@j-carl I was about to write my own module until I found yours. Seems to work as expected, but shouldn't all new AWS modules be written using Boto3 instead of boto?

@j-carl
Contributor
j-carl commented May 18, 2016

@linuxdynasty you right, but I wrote these modules about one year ago. I think at that point boto3 wasn't fully supported within ansible.

@bryanhiestand

I'm not sure why this is still in community_review after nearly two years, but this has been working for me for a while now.

@kylelundstedt

Ditto @bryanhiestand; love to see these Redshift modules moved out of community_review!

@defionscode
Member

@linuxdynasty this isn't considered "new" as it pertains to the boto3 only rule for new submissions. This was submitted well before the boto3 mandate. I would be okay with this being merged in. cc/ @ryansb

@ryansb
Member
ryansb commented Sep 15, 2016

Going to consider the comments above about folks using this module as-is as shipits. shipit

@gregdek
Contributor
gregdek commented Sep 15, 2016

Thanks again to @j-carl for this PR, and thanks @ansible/core for reviewing. Marking for inclusion.

[This message brought to you by your friendly Ansibull-bot.]

@gregdek gregdek added shipit and removed community_review labels Sep 15, 2016
@gundalow gundalow added a commit to ansible/ansibullbot that referenced this pull request Sep 16, 2016
@gundalow gundalow cloud/amazon/redshift.py -> j-carl e60738c
@gundalow gundalow merged commit 9b5c64e into ansible:devel Sep 16, 2016
@gundalow
Contributor

Merged, thanks.

@slaingnials slaingnials added a commit to slaingnials/ansible-modules-extras that referenced this pull request Oct 24, 2016
@j-carl @slaingnials j-carl + slaingnials New ansible module for aws Redshift and Redshift subnet group (#185) f559c68
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment