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

[Plugin elasticsearch_info] Create a plugin to describe elasticsearch domains #647

Closed

Conversation

MKCG
Copy link
Contributor

@MKCG MKCG commented Jul 18, 2021

SUMMARY

Introduce a new plugin to describe AWS Elasticsearch domains

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

elasticsearch_info

ADDITIONAL INFORMATION

There is a actually not module to describe Elasticsearch domains so here it is.

- name: Retrieve informations about a specific Elasticsearch domain
  community.aws.elasticsearch_info:
    domain_name: app_domain

- name: Retrieve informations about all Elasticsearch domains
  community.aws.elasticsearch_info:

I don't know yet how to create and run integration tests on this project so none is included at the moment and I will be more than glad to have some help on this part.

@ansibullbot
Copy link

@ansibullbot ansibullbot added community_review module module needs_triage new_contributor Help guide this first time contributor new_module New module new_plugin New plugin plugins plugin (any type) labels Jul 18, 2021
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

In general the code looks good. A couple of minor inline comments.

Additionally,

DOCUMENTATION = '''
---
module: elasticsearch_info
version_added: 1.6.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
version_added: 1.6.0
version_added: 2.0.0

It's currently expected that 2.0.0 will be the next release

)

module = AnsibleAWSModule(argument_spec=argument_spec, supports_check_mode=True)
client = module.client('es')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
client = module.client('es')
client = module.client('es', retry_decorator=AWSRetry.jittered_backoff())

https://docs.ansible.com/ansible/latest/dev_guide/platforms/aws_guidelines.html#api-throttling-rate-limiting-and-pagination

We also have a decorator which makes it easier to switch out the retry configuration when appropriate. (Often more helpful when modules manipulate resources).

You can then enable the retries on each client call with something like

     domain_names = client.list_domain_names(aws_retry=True)['DomainNames'] 

(aws_retry is optional and defaults to False)

Personally I have a slight preference for the jittered backoff: https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/ but the exponential is fine as is the @ mechanism for enabling them

@tremble
Copy link
Contributor

tremble commented Jul 18, 2021

I just re-read your initial comment about not knowing where to find the integration tests:

There's a (minimal) integration test for elasticache
https://github.com/ansible-collections/community.aws/tree/main/tests/integration/targets/elasticache

They're basically just Ansible roles.

@mohag
Copy link

mohag commented Aug 23, 2021

(It seems like the ElasticSearch service might be renamed to OpenSearch) (That has not happened yet though)

alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request May 25, 2022
ec2_vpc_igw: fix 'NoneType' object is not subscriptable

Depends-On: ansible/ansible-zuul-jobs#1368
SUMMARY
Add "InternetGatewayAttached" waiter
Fixes ansible-collections#647
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
ec2_vpc_igw

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Jill R <None>
Reviewed-by: Abhijeet Kasurde <None>
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request May 25, 2022
`ec2_vpc_igw`: fix NoneType error

SUMMARY

use paginator for describe internet gateways and add retry_codes='InvalidInternetGatewayID.NotFound' (thanks @alinabuzachis)
Fixes ansible-collections#647

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
ec2_vpc_igw
ADDITIONAL INFORMATION

previously added InternetGatewayAttached waiter but problem still persists (#691)

Reviewed-by: Abhijeet Kasurde <None>
Reviewed-by: Jill R <None>
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request May 25, 2022
…sible-collections#766)

ec2_vpc_igw - dont user filters to paginate to fix NoneType error

Depends-On: ansible/ansible-zuul-jobs#1498
SUMMARY
Use gateway id when describing gateway instances as opposed to filters if possible to avoid bug of no gateways found.
Fixes ansible-collections#647
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
ec2_vpc_igw

Reviewed-by: Gonéri Le Bouder <goneri@lebouder.net>
Reviewed-by: Mark Chappell <None>
Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Alina Buzachis <None>
@tremble
Copy link
Contributor

tremble commented Jul 1, 2022

Thanks for taking the time to submit this PR.

It looks like elasticsearch did indeed get renamed to "opensearch". We recently released v4.0.0 of this collection which includes support for managing and describing opensearch clusters.

As such I'm going to close out this PR.

@tremble tremble closed this Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community_review module module new_contributor Help guide this first time contributor new_module New module new_plugin New plugin plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants