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

AWS Redshift: fix parameters check and port module to boto3 #37052

Open
wants to merge 22 commits into
base: devel
from

Conversation

@rafaeldriutti

rafaeldriutti commented Mar 6, 2018

SUMMARY

Make it work with current ansible and boto

Fixes #37051

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

redshift

ANSIBLE VERSION
$ ansible --version
ansible 2.6.0 (devel e3895e4d83) last updated 2018/03/05 21:53:15 (GMT -300)
  config file = None
  configured module search path = [u'/home/rdriutti/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/rdriutti/code/ansible/lib/ansible
  executable location = /home/rdriutti/code/ansible/bin/ansible
  python version = 2.7.14 (default, Sep 23 2017, 22:06:14) [GCC 7.2.0]
ADDITIONAL INFORMATION

See https://github.com/boto/boto/blob/2.48.0/boto/redshift/layer1.py#L313 for the enhanced_vpc_routing thing.
More info on the bug report.

@ansibot

This comment has been minimized.

Contributor

ansibot commented Mar 6, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Mar 6, 2018

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

lib/ansible/modules/cloud/amazon/redshift.py:327:72: undefined-variable Undefined variable 'idnetifier'
lib/ansible/modules/cloud/amazon/redshift.py:501:31: singleton-comparison Comparison to False should be 'not expr' or 'expr is False'

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

lib/ansible/modules/cloud/amazon/redshift.py:282:15: E201 whitespace after '('
lib/ansible/modules/cloud/amazon/redshift.py:283:15: E128 continuation line under-indented for visual indent
lib/ansible/modules/cloud/amazon/redshift.py:284:15: E128 continuation line under-indented for visual indent
lib/ansible/modules/cloud/amazon/redshift.py:285:15: E128 continuation line under-indented for visual indent
lib/ansible/modules/cloud/amazon/redshift.py:286:15: E128 continuation line under-indented for visual indent
lib/ansible/modules/cloud/amazon/redshift.py:287:15: E128 continuation line under-indented for visual indent
lib/ansible/modules/cloud/amazon/redshift.py:288:15: E128 continuation line under-indented for visual indent
lib/ansible/modules/cloud/amazon/redshift.py:289:15: E128 continuation line under-indented for visual indent
lib/ansible/modules/cloud/amazon/redshift.py:363:13: E128 continuation line under-indented for visual indent
lib/ansible/modules/cloud/amazon/redshift.py:390:86: E202 whitespace before ')'
lib/ansible/modules/cloud/amazon/redshift.py:422:5: E303 too many blank lines (2)
lib/ansible/modules/cloud/amazon/redshift.py:429:88: E202 whitespace before ')'
lib/ansible/modules/cloud/amazon/redshift.py:434:84: E202 whitespace before ')'
lib/ansible/modules/cloud/amazon/redshift.py:449:87: E202 whitespace before ')'
lib/ansible/modules/cloud/amazon/redshift.py:501:60: E712 comparison to False should be 'if cond is False:' or 'if not cond:'

click here for bot help

@ansibot

This comment has been minimized.

Contributor

ansibot commented Mar 6, 2018

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

lib/ansible/modules/cloud/amazon/redshift.py:363:13: E128 continuation line under-indented for visual indent
lib/ansible/modules/cloud/amazon/redshift.py:390:86: E202 whitespace before ')'

click here for bot help

@ryansb ryansb removed the needs_triage label Mar 6, 2018

@ryansb ryansb requested review from ryansb and s-hertel Mar 6, 2018

@ryansb ryansb changed the title from fix parameters check and port module to boto3 - Fixes #37051 to AWS Redshift: fix parameters check and port module to boto3 - Fixes #37051 Mar 6, 2018

@ryansb ryansb changed the title from AWS Redshift: fix parameters check and port module to boto3 - Fixes #37051 to AWS Redshift: fix parameters check and port module to boto3 Mar 6, 2018

@willthames

This comment has been minimized.

Contributor

willthames commented Mar 6, 2018

This change will require a full test suite as per our policy: https://github.com/ansible/community/blob/master/group-aws/integration.md#policy

Documentation on providing a test suite is at: https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/cloud/amazon/GUIDELINES.md#integration-tests-for-aws-modules

Let us know if there's any help we can provide. A good starting model is the ec2_group test suite (test/integration/targets/ec2_group)

@ansibot

This comment has been minimized.

Contributor

ansibot commented Sep 10, 2018

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

test/integration/targets/redshift/aliases:0:0: missing alias `shippable/aws/group[1-2]` or `unsupported`

click here for bot help

@ansibot ansibot added the ci_verified label Sep 10, 2018

@ansibot ansibot removed the ci_verified label Sep 10, 2018

@s-hertel

@rafaeldriutti Thanks for all the work you've put into this, it's looking good! Feel free to add yourself to the author list if you'd like to help maintain and merge things from the community for the module in the future.

region, ec2_url, aws_connect_params = get_aws_connection_info(module)

This comment has been minimized.

@s-hertel

s-hertel Sep 10, 2018

Contributor

You can remove some connection boilerplate (get_aws_connection_info, if not region:, boto3_conn(..) and replace it with conn = module.client('redshift')

@@ -434,8 +441,8 @@ def main():
'dw2.8xlarge'], required=False),
username=dict(required=False),
password=dict(no_log=True, required=False),
db_name=dict(require=False),
cluster_type=dict(choices=['multi-node', 'single-node', ], default='single-node'),
db_name=dict(require=False, type='str'),

This comment has been minimized.

@s-hertel

s-hertel Sep 10, 2018

Contributor

type='str' is the default so you can omit it if you want. Typically the non-defaults are listed for AWS modules.

@@ -276,36 +281,40 @@ def create_cluster(module, redshift):
'number_of_nodes', 'publicly_accessible',
'encrypted', 'elastic_ip', 'enhanced_vpc_routing'):
if p in module.params:
params[p] = module.params.get(p)
# https://github.com/boto/boto3/issues/400
if module.params.get(p) is not None:

This comment has been minimized.

@s-hertel

s-hertel Sep 10, 2018

Contributor

Rather than doing if p in module.params: on line 283 you could move this line if module.params.get(p) is not None: there and unindent the line below.

resource = redshift.describe_clusters(identifier)['DescribeClustersResponse']['DescribeClustersResult']['Clusters'][0]
except boto.exception.JSONResponseError as e:
module.fail_json(msg=str(e))
resource = redshift.describe_clusters(ClusterIdentifier=identifier)['Clusters'][0]

This comment has been minimized.

@s-hertel

s-hertel Sep 10, 2018

Contributor

Should retrieving the resource happen after waiting if wait is True? It seems like this could fail sometimes.

except boto.exception.JSONResponseError as e:
module.fail_json(msg=str(e))
redshift.modify_cluster(ClusterIdentifier=identifier,

This comment has been minimized.

@s-hertel

s-hertel Sep 10, 2018

Contributor

I'm not sure why an error describing the cluster leads to trying to modify the cluster. What is the exception expected here?

module.fail_json(msg=str(e))
resource = redshift.describe_clusters(ClusterIdentifier=identifier)['Clusters'][0]
except (botocore.exceptions.BotoCoreError, botocore.exceptions.ClientError) as e:
module.fail_json_aws(e, msg="Failed to create cluster")

This comment has been minimized.

@s-hertel

s-hertel Sep 10, 2018

Contributor

"Failed to describe cluster" would be a more accurate error message.

@@ -244,6 +240,14 @@ def _collect_facts(resource):
facts['public_ip_address'] = node['PublicIPAddress']
break
# Some parameters are not ready instantly if you don't wait for available

This comment has been minimized.

@s-hertel

s-hertel Sep 10, 2018

Contributor

Should you default these values to None that way the returned keys are always the same? It would also be good to add this note to the return docs for these four.

@@ -0,0 +1,3 @@
cloud/aws
posix/ci/cloud/group4/aws
shippable/aws/group1

This comment has been minimized.

@s-hertel

s-hertel Sep 10, 2018

Contributor

I think these take too long to run in CI... 25 minutes and counting. They should probably have the unsupported alias added and people will need to run them manually before merging redshift things.

Update - they took 34 minutes, which is not the worst but is still probably too slow.

This comment has been minimized.

@rafaeldriutti

rafaeldriutti Sep 19, 2018

I can make it faster by removing the multi-node creation and delete test, that adds like 10-15 mins to the run.

- Rework modify function.
- Default unavaliable parameters to none.
- Add cluster modify test
@ansibot

This comment has been minimized.

Contributor

ansibot commented Sep 18, 2018

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

lib/ansible/modules/cloud/amazon/redshift.py:246:15: singleton-comparison Comparison to False should be 'not expr' or 'expr is False'

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

lib/ansible/modules/cloud/amazon/redshift.py:128:161: E501 line too long (165 > 160 characters)
lib/ansible/modules/cloud/amazon/redshift.py:246:46: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
lib/ansible/modules/cloud/amazon/redshift.py:443:5: E303 too many blank lines (2)

click here for bot help

@rafaeldriutti

This comment has been minimized.

rafaeldriutti commented Sep 24, 2018

How can I add the ModifyCluster permission to the CI policy?
An error occurred (AccessDenied) when calling the ModifyCluster operation: User: arn:aws:sts::966509639900:assumed-role/ansible-core-ci-test-prod/prod=shippable=ansible=ansible=84814.66 is not authorized to perform: redshift:ModifyCluster on resource: arn:aws:redshift:us-east-1:966509639900:cluster:shippable-84814-66"

@mattclay

This comment has been minimized.

Member

mattclay commented Sep 24, 2018

@rafaeldriutti Adding permissions to CI is something that someone on the Ansible Core Team will need to do. This PR is labeled needs_ci_update, so it's on the list of PRs which are blocked on CI permissions.

@ansibot ansibot removed the stale_ci label Nov 12, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 15, 2018

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

lib/ansible/modules/cloud/amazon/redshift.py:316:12: duplicate-except Catching previously caught exception type ClientError
lib/ansible/modules/cloud/amazon/redshift.py:380:12: duplicate-except Catching previously caught exception type ClientError

click here for bot help

@ansibot ansibot added the stale_ci label Nov 23, 2018

@rafaeldriutti

This comment has been minimized.

rafaeldriutti commented Dec 5, 2018

Thanks for all the help @s-hertel 👍 Seems the linter doesn't like the additional exeption handling added on b96675f.

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