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

dms_replication_subnet_group #56980

Open
wants to merge 7 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@ruimoreira
Copy link
Contributor

commented May 26, 2019

SUMMARY

New module for dms_replication_subnet_group

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

dms_replication_subnet_group

ADDITIONAL INFORMATION
@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 26, 2019

@BobBoldin @Constantin007 @Constantin07 @Deepakkothandan @Etherdaemon @Java1Guy @Madhura-CSI @MichaelBaydoun @Sodki @Zeekin @adq @akazakov @alachaum @amir343 @anryko @bekelchik @brandond @captainkerk @chenl87 @defunctio @dennisconrad @dkhenry @fiunchinho @fivethreeo @flowerysong @garethr @gobins @gunzy83 @gurumaia @hsingh @hyperized @iiibrad @infectsoldier @j-carl @jarv @jimbydamonk @jmenga @joelthompson @jonhadfield @jonmer85 @joshsouza @jsdalton @jsmartin @kaczynskid @leedm777 @linuxdynasty @loia @lwade @michaeljs1990 @minichate @mjschultz @mmochan @nand0p @naslanidis @nathanwebsterdotme @nerzhul @nickball @orthanc @ozbillwang @piontas @pjodouin @prasadkatti @psykotox @ptux @pwnall @raags @rafaeldriutti @rickmendes @roadmapper @rrey @ryansydnor @scicoin-project @scottanderson42 @sdubrul @shepdelacreme @silviud @slapula @steynovich @tastychutney @tgerla @timmahoney @tomislacker @tsiganenok @viper233 @whiter @willricardo @wilvk @wimnat @yaakov-github @zacblazic @zbal @zimbatm

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 26, 2019

@ruimoreira, 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

@jillr
Copy link
Contributor

left a comment

Thanks for this module @ruimoreira . The tests are not passing for me locally but I believe it's unrelated to this change, I will look at them again later this week.

# ReplicationSubnetGroupIdentifier gets translated to lower case anyway by the API
ReplicationSubnetGroupIdentifier=module.params.get('identifier').lower(),
ReplicationSubnetGroupDescription=module.params.get('description'),
SubnetIds=module.params.get('subnetids'),

This comment has been minimized.

Copy link
@jillr

jillr Jun 17, 2019

Contributor
Suggested change
SubnetIds=module.params.get('subnetids'),
SubnetIds=module.params.get('subnet_ids'),

and here as well

description:
- The description for the subnet group.
type: str
subnetids:

This comment has been minimized.

Copy link
@jillr

jillr Jun 17, 2019

Contributor
Suggested change
subnetids:
subnet_ids:

Snake casing would help with readability of this option.

exit_message = None
changed = False
if not HAS_BOTO3:

This comment has been minimized.

Copy link
@jillr

jillr Jun 17, 2019

Contributor
Suggested change

nit, but please remove this extra line break.

Show resolved Hide resolved test/integration/targets/dms_replication_subnet_group/tasks/main.yml
az: eu-west-1c
register: subnet2

- name: create replication subnet subnet group

This comment has been minimized.

Copy link
@jillr

jillr Jun 17, 2019

Contributor
Suggested change
- name: create replication subnet subnet group
- name: create replication subnet group
region: "{{ aws_region }}"
resource_prefix: "test_dms_sg"
dms_sg_identifier: "{{ resource_prefix }}-dms"
no_log: no

This comment has been minimized.

Copy link
@jillr

jillr Jun 17, 2019

Contributor
Suggested change
no_log: no
no_log: yes
@ruimoreira

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

Hello @jillr

I will make these changes shortly, thank you for your help.

- renaming subnetids to subnet_ids
- formattig issues
- fixing lack of credentials in test

@ansibot ansibot removed the stale_ci label Jun 19, 2019

ruimoreira added some commits Jun 19, 2019

@ruimoreira

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

Hello @jillr

Your changes have been committed. Thank you for your help.

if paramname in param_described.keys() and \
param_described.get(paramname) == modparams[paramname]:
pass
elif paramname == 'subnet_ids':

This comment has been minimized.

Copy link
@jillr

jillr Jun 20, 2019

Contributor
Suggested change
elif paramname == 'subnet_ids':
elif paramname == 'SubnetIds':
# ReplicationSubnetGroupIdentifier gets translated to lower case anyway by the API
ReplicationSubnetGroupIdentifier=module.params.get('identifier').lower(),
ReplicationSubnetGroupDescription=module.params.get('description'),
subnet_ids=module.params.get('subnet_ids'),

This comment has been minimized.

Copy link
@jillr

jillr Jun 20, 2019

Contributor
Suggested change
subnet_ids=module.params.get('subnet_ids'),
SubnetIds=module.params.get('subnet_ids'),

Here this var needs to stay as you had it originally - ansible prefers snake cased but boto typically needs camelcase. instance_parameters will be passed into the boto connection so needs to match what the API expects, both here and later when you access the returned parameters.
https://docs.aws.amazon.com/dms/latest/APIReference/API_CreateReplicationSubnetGroup.html#API_CreateReplicationSubnetGroup_RequestSyntax

subnets = []
for subnet in param_described.get('Subnets'):
subnets.append(subnet.get('SubnetIdentifier'))
for modulesubnet in modparams['subnet_ids']:

This comment has been minimized.

Copy link
@jillr

jillr Jun 20, 2019

Contributor
Suggested change
for modulesubnet in modparams['subnet_ids']:
for modulesubnet in modparams['SubnetIds']:

This comment has been minimized.

Copy link
@jillr

jillr Jun 24, 2019

Contributor

This one still needs to be swapped.

Show resolved Hide resolved test/integration/targets/dms_replication_subnet_group/tasks/main.yml
@jillr
Copy link
Contributor

left a comment

Thanks for those updates, just a couple more comments.

- name: create replication subnet group
dms_replication_subnet_group:
state: present
identifier: "dev-sngroup"

This comment has been minimized.

Copy link
@jillr

jillr Jun 24, 2019

Contributor
Suggested change
identifier: "dev-sngroup"
identifier: "{{ dms_sg_identifier }}"
dms_replication_subnet_group:
state: present
identifier: "dev-sngroup"
description: "Development Subnet Group asdasdas"

This comment has been minimized.

Copy link
@jillr

jillr Jun 24, 2019

Contributor
Suggested change
description: "Development Subnet Group asdasdas"
description: "Development Subnet Group"

The name and description in this task and the ones in "name: create subnet group no change" are different, so the no-change assert is failing.

subnets = []
for subnet in param_described.get('Subnets'):
subnets.append(subnet.get('SubnetIdentifier'))
for modulesubnet in modparams['subnet_ids']:

This comment has been minimized.

Copy link
@jillr

jillr Jun 24, 2019

Contributor

This one still needs to be swapped.

- fixed task name in integration tests
- fixed modparam name
@ruimoreira

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

Thank you so much for your help in this.
This should hopefully now pass the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.