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

Adding module that manages graph database clusters in AWS Neptune #41015

Open
wants to merge 11 commits into
base: devel
from

Conversation

@slapula
Contributor

slapula commented Jun 1, 2018

SUMMARY

AWS just released a new managed graph database service called AWS Neptune. I had some time last night to play around with the API and it's similar to RDS and Redshift. This PR adds a module that manages graph database clusters in Neptune. Like my Secrets Manager module (#40093), this one also requires botocore>=1.10.0. This will likely be the first of several modules I will be writing for Neptune.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

aws_neptune_cluster

ANSIBLE VERSION
ansible 2.4.1.0
  config file = None
  configured module search path = [u'/Users/asmith/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/asmith/.pyenv/versions/2.7.13/lib/python2.7/site-packages/ansible
  executable location = /Users/asmith/.pyenv/versions/2.7.13/bin/ansible
  python version = 2.7.13 (default, Sep  8 2017, 15:16:38) [GCC 4.2.1 Compatible Apple LLVM 8.1.0 (clang-802.0.42)]
@ansibot

This comment has been minimized.

Contributor

ansibot commented Jun 1, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Jun 1, 2018

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

lib/ansible/modules/cloud/amazon/neptune_cluster.py:180:29: undefined-variable Undefined variable 'e'

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Jun 1, 2018

lib/ansible/modules/cloud/amazon/neptune_cluster.py Outdated
return False
def cluster_ready(client, module, params):

This comment has been minimized.

@s-hertel
lib/ansible/modules/cloud/amazon/neptune_cluster.py Outdated
cluster_ready(client, module, params)
result['db_cluster_arn'] = response['DBCluster']['DBClusterArn']
result['changed'] = True
return result

This comment has been minimized.

@s-hertel

s-hertel Jun 1, 2018

Contributor

You can omit this return since it will be returned on line 238.

lib/ansible/modules/cloud/amazon/neptune_cluster.py Outdated
params['FinalDBSnapshotIdentifier'] = module.params.get('final_snapshot_id')
response = client.delete_db_cluster(**params)
result['changed'] = True
return result

This comment has been minimized.

@s-hertel

s-hertel Jun 1, 2018

Contributor

This is being returned below too.

lib/ansible/modules/cloud/amazon/neptune_cluster.py Outdated
result['current_config'] = response['DBClusters'][0]
result['db_cluster_arn'] = response['DBClusters'][0]['DBClusterArn']
return True
except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError):

This comment has been minimized.

@s-hertel

s-hertel Jun 1, 2018

Contributor

Returning False for any of these exceptions can mask important errors. Can you just catch the one specific to the DB cluster not existing to return False and other exceptions call fail_json_aws? Here's an example from another module for how to handle a specific exception code differently:

except conn.exceptions.from_code('DBInstanceNotFound'):
results = []
except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError) as e:
module.fail_json_aws(e, "Couldn't get instance information")

lib/ansible/modules/cloud/amazon/neptune_cluster.py Outdated
if desired_state == 'present':
if not cluster_status:
create_cluster(client, module, params, result)

This comment has been minimized.

@s-hertel

s-hertel Jun 1, 2018

Contributor

Rather than initialize result above and then passing it around and modifying the contents it would be more clear to return a result dict created inside create_cluster (and update_cluster and delete_cluster) return {'db_cluster_arn': response['DBCluster']['DBClusterArn'], 'changed': True} and result = create_cluster(client, module, params) (because right now the return statements do nothing), etc

@s-hertel s-hertel removed the needs_triage label Jun 1, 2018

lib/ansible/modules/cloud/amazon/neptune_cluster.py Outdated
from ansible.module_utils.aws.core import AnsibleAWSModule
from ansible.module_utils.ec2 import boto3_conn, get_aws_connection_info, AWSRetry
from ansible.module_utils.ec2 import camel_dict_to_snake_dict, boto3_tag_list_to_ansible_dict

This comment has been minimized.

@s-hertel

s-hertel Jun 1, 2018

Contributor

This and the line above are unused imports

lib/ansible/modules/cloud/amazon/neptune_cluster.py Outdated
DOCUMENTATION = r'''
---
module: neptune_cluster

This comment has been minimized.

@s-hertel

s-hertel Jun 1, 2018

Contributor

Maybe aws_neptune_cluster

@slapula

This comment has been minimized.

Contributor

slapula commented Jun 2, 2018

@s-hertel Aside from the module re-name, I believe I've dealt with all of your comments. Thanks for the feedback! I even included custom cluster_available and cluster_deleted waiters like you recommended.

lib/ansible/modules/cloud/amazon/neptune_cluster.py Outdated
description:
- Create, modify, and destroy graph database clusters on AWS Neptune
version_added: "2.7"
requirements: [ 'botocore>=1.10.0', 'boto3' ]

This comment has been minimized.

@s-hertel

s-hertel Jun 7, 2018

Contributor

I think the minimum botocore is 1.10.30

lib/ansible/modules/cloud/amazon/neptune_cluster.py Outdated
response = client.describe_db_clusters(
DBClusterIdentifier=params['DBClusterIdentifier']
)
except client.exceptions.from_code('DBClusterNotFound'):

This comment has been minimized.

@s-hertel

s-hertel Jun 7, 2018

Contributor

Sorry, I was mistaken about this, it masks unwanted things. Catching ClientError exceptions and as e and then checking e.response['Error']['Code'] is necessary.

test/runner/requirements/constraints.txt Outdated
@@ -17,3 +17,4 @@ ntlm-auth >= 1.0.6 # message encryption support
requests-ntlm >= 1.1.0 # message encryption support
requests-credssp >= 0.1.0 # message encryption support
voluptuous >= 0.11.0 # Schema recursion via Self
botocore >= 1.10.0 # adds support for the following AWS services: secretsmanager, fms, acm-pca, neptune

This comment has been minimized.

@s-hertel

s-hertel Jun 7, 2018

Contributor

Not necessary to add this here. This will mean people not running AWS tests will also install botocore. There's an AWS requirements file which lists boto3, which also installs botocore.

This comment has been minimized.

@slapula

slapula Jun 7, 2018

Contributor

Understood! That line has been removed.

@s-hertel s-hertel force-pushed the slapula:aws_neptune_cluster_module branch 2 times, most recently Jun 7, 2018

@s-hertel

This comment has been minimized.

Contributor

s-hertel commented Jun 7, 2018

Is there a reason not to rename it? It would be more consistent with other new AWS modules. Besides that this looks good to me. I expect that it will fail CI right now due to lack of permissions but I'll work on getting those added this week or next.

@s-hertel

This comment has been minimized.

Contributor

s-hertel commented Jun 7, 2018

Thanks for adding the waiters, btw. The botocore repo may be interested in those as well.

@ansibot ansibot removed the needs_ci label Jun 7, 2018

lib/ansible/modules/cloud/amazon/aws_neptune_cluster.py Outdated
def cluster_exists(client, module, params):
if module.check_mode and module.params.get('state') == 'absent':

This comment has been minimized.

@s-hertel

s-hertel Jun 7, 2018

Contributor

Oops, missed something. I think this should still return the describe_db_clusters call so changed is accurately reflected by whether or not the cluster exists. No changes are being made by that call so it's fine and you're checking for module.check_mode in delete_cluster().

This comment has been minimized.

@slapula

slapula Jun 7, 2018

Contributor

Ok, makes sense to me! Updated.

@ansibot

This comment has been minimized.

Contributor

ansibot commented Jun 7, 2018

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

test/runner/requirements/integration.cloud.aws.txt:3:9: put constraints in `test/runner/requirements/constraints.txt`

click here for bot help

test/runner/requirements/integration.cloud.aws.txt Outdated
@@ -1,2 +1,3 @@
boto
boto3
botocore>=1.10.30

This comment has been minimized.

@mattclay

mattclay Jun 7, 2018

Member

Requirements such as botocore should be listed in this file, but version constraints such as botocore >= 1.10.30 should be in constraints.txt instead. Be sure to include a comment explaining the reason for the constraint.

@ansibot ansibot added the stale_ci label Jun 16, 2018

@s-hertel

Looks great, thanks for the time you spent on this!

lib/ansible/modules/cloud/amazon/aws_neptune_cluster.py Outdated
'db_engine_version': dict(type='str', choices=['1.0.1']),
'port': dict(type='int', default=8182),
'master_username': dict(type='str'),
'master_password': dict(type='str'),

This comment has been minimized.

@s-hertel

s-hertel Jul 19, 2018

Contributor

You should set no_log=True for this

@s-hertel s-hertel force-pushed the slapula:aws_neptune_cluster_module branch to ec7d87e Jul 19, 2018

@@ -17,3 +17,4 @@ ntlm-auth >= 1.0.6 # message encryption support
requests-ntlm >= 1.1.0 # message encryption support
requests-credssp >= 0.1.0 # message encryption support
voluptuous >= 0.11.0 # Schema recursion via Self
botocore >= 1.10.30 # adds support for the following AWS services: secretsmanager, fms, acm-pca, neptune

This comment has been minimized.

@ryansb

ryansb Jul 20, 2018

Contributor

I think this affects all integration tests. Instead, I think it belongs in integration.cloud.aws.txt. @mattclay can you confirm?

This comment has been minimized.

@mattclay

mattclay Jul 20, 2018

Member

This is the correct place to put version constraints. It affects all tests, but will not require the package to be installed for all tests. It will, however, require the version constraint be met whenever the package is installed.

Package requirements (only the package name) should be in the appropriate requirements file, such as integration.cloud.aws.txt. However, package constraints (version number requirements) should be placed in constraints.txt. This avoids having conflicting version requirements across tests while still allowing different tests to require different packages.

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