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

elbv2 boto3 client - implement new api for ip address type #310

Merged
merged 6 commits into from
Apr 19, 2021

Conversation

abikouo
Copy link
Contributor

@abikouo abikouo commented Mar 25, 2021

SUMMARY

implement boto3 api for elbv2 client to set ip address type on application and network load balancing

ansible-collections/community.aws#499

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

This API will be used for elb_application_xxx and elb_network_xxx modules


@abikouo abikouo changed the title implement new boto3 api to set ip address type elbv2 boto3 client - implement new api for ip address type Mar 25, 2021
@ansibullbot ansibullbot added feature This issue/PR relates to a feature request module_utils module_utils needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR needs_triage plugins plugin (any type) labels Mar 25, 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.

Thanks for taking the time to raise this PR. You've fallen foul of the sanity tests (mostly white-space related).

Please add a changelog fragment ( https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs-how-to ) module_utils and their APIs are as much a part of the collection as modules.

Additionally, I don't think it's necessary to make the extra describe API call, self.elb should already include this information.

abikouo and others added 2 commits March 29, 2021 11:41
Co-authored-by: Mark Chappell <mchappel@redhat.com>
@abikouo abikouo removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR needs_triage labels Mar 30, 2021
@abikouo abikouo requested review from tremble and removed request for gravesm March 30, 2021 10:30
@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Mar 30, 2021
@tremble
Copy link
Contributor

tremble commented Mar 30, 2021 via email

Copy link
Contributor

@jillr jillr left a comment

Choose a reason for hiding this comment

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

Thanks for this patch @abikouo. Is there a module bug or feature request that this change corresponds to? It would be good to understand how the new feature will be used so we can test it.

Could you please add unit tests for the new functionality to tests/unit/module_utils/test_elbv2.py? We don't have the entire utility covered yet (we didn't always require tests for AWS) but for anything new we try to add coverage so there's fewer gaps to have to cover later.

@abikouo
Copy link
Contributor Author

abikouo commented Apr 7, 2021

Thanks for this patch @abikouo. Is there a module bug or feature request that this change corresponds to? It would be good to understand how the new feature will be used so we can test it.

Could you please add unit tests for the new functionality to tests/unit/module_utils/test_elbv2.py? We don't have the entire utility covered yet (we didn't always require tests for AWS) but for anything new we try to add coverage so there's fewer gaps to have to cover later.

Thanks @jillr for the feedback
This api is going to be used for the elb_application_xxx and elb_network_xxx modules, full details in the following PR
ansible-collections/community.aws#499

I will implement the unit tests for this module.

@ansibullbot ansibullbot added community_review tests tests and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Apr 14, 2021
Copy link
Contributor

@jillr jillr left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @abikouo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community_review feature This issue/PR relates to a feature request module_utils module_utils plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants