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

#223 Port rds_subnet_group to boto3 #224

Merged
merged 11 commits into from
Sep 10, 2020
Merged

#223 Port rds_subnet_group to boto3 #224

merged 11 commits into from
Sep 10, 2020

Conversation

pm98zz-c
Copy link
Contributor

@pm98zz-c pm98zz-c commented Sep 7, 2020

SUMMARY

Use boto3 instead of boto (and remove logic on mandatory arguments on state: absent)

ISSUE TYPE
  • Chore
COMPONENT NAME

rds_subnet_group.py

ADDITIONAL INFORMATION

I've only focused on the minimal work to get it working in the same way as it was from a functional point of view. Would probably need some tests and a few additional features (like support for tags), but seems to be working for my use case.

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.

Thank you for taking the time to port this code over to boto3, it's very much appreciated. Something went wrong with the integration tests so they didn't run properly the second time, but I suspect they're going to throw up some of the issues in my review.

Additionally, please would you add a changelog entry: https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs-how-to

@pm98zz-c
Copy link
Contributor Author

pm98zz-c commented Sep 7, 2020

Thanks the prompt review.

Something went wrong with the integration tests so they didn't run properly the second time, but I suspect they're going to throw up some of the issues in my review.

Fairly sure I would had missed some parts. For example, I noticed afterward the existing module does'nt have an ANSIBLE_METADATA block, so I'll probably need to add one. Not sure neither about the formatting (opened ansible/community#564 around that).

Additionally, please would you add a changelog entry: https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs-how-to

Will do.

@pm98zz-c
Copy link
Contributor Author

pm98zz-c commented Sep 7, 2020

Right, end of day for me here, but I'll try to get this done in the coming days. Thanks.

@tremble
Copy link
Contributor

tremble commented Sep 7, 2020

https://app.shippable.com/github/ansible-collections/community.aws/runs/705/summary/console

It looks like the tests will need some minor tweaks (the error messages have changed) you can find them in tests/integration/targets/rds_subnet_group

@tremble
Copy link
Contributor

tremble commented Sep 7, 2020

ANSIBLE_METADATA is no longer needed, it went away as a part of the big split: ansible-collections/overview#57

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.

Looking good, while you're at it a couple of minor suggestions.

@pm98zz-c pm98zz-c marked this pull request as draft September 8, 2020 10:38
@pm98zz-c
Copy link
Contributor Author

pm98zz-c commented Sep 8, 2020

Right, tests are still failing (and I messed up the document "sample" part). I'll need to setup properly and re-run all that. Leaving as draft until I've resolved it.

@pm98zz-c
Copy link
Contributor Author

pm98zz-c commented Sep 8, 2020

@tremble Right, build failed as something timed out, I don't think I can re-trigger it myself.

@pm98zz-c pm98zz-c requested a review from tremble September 8, 2020 15:13
@pm98zz-c pm98zz-c marked this pull request as ready for review September 8, 2020 15:14
@ansibullbot
Copy link

@pm98zz-c this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibullbot ansibullbot added affects_2.10 merge_commit This PR contains at least one merge commit. Please resolve! module module needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor labels Sep 9, 2020
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.

  • Code looks sane
  • Tests are passing
  • Changes to the tests are non-breaking
    • Changes to the error messages
    • Being more relaxed about options when state=absent

LGTM

@tremble tremble merged commit c19d479 into ansible-collections:main Sep 10, 2020
@tremble
Copy link
Contributor

tremble commented Sep 10, 2020

Thank-you for your contribution. That's one less boto v2 based module.

@pm98zz-c
Copy link
Contributor Author

Cheers, thanks for your patience. First time around, it'll be more straightforward next time!

alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
…ctions#224)

* Port rds_subnet_group to boto3
* Linting fixes
* Add more meaningful error messages, add changelog fragment
* Remove test on mandatory args for state absent
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
…ctions#224)

* Port rds_subnet_group to boto3
* Linting fixes
* Add more meaningful error messages, add changelog fragment
* Remove test on mandatory args for state absent
EmlynK pushed a commit to codeenigma/ce-provision that referenced this pull request Sep 15, 2021
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request May 25, 2022
…s#224)

Initial integration tests for ec2_vpc_dhcp_option. Based on work started
in ansible-collections#109

Co-authored-by: s-hertel <[email protected]>
abikouo pushed a commit to abikouo/community.aws that referenced this pull request Oct 24, 2023
…ctions#224)

* Port rds_subnet_group to boto3
* Linting fixes
* Add more meaningful error messages, add changelog fragment
* Remove test on mandatory args for state absent

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections@c19d479
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.10 merge_commit This PR contains at least one merge commit. Please resolve! module module needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants