-
Notifications
You must be signed in to change notification settings - Fork 398
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
elb_classic_lb_info: Refactor elb_classic_lb_info module #2139
elb_classic_lb_info: Refactor elb_classic_lb_info module #2139
Conversation
be26fd2
to
acc5fa8
Compare
Docs Build 📝Thank you for contribution!✨ This PR has been merged and your docs changes will be incorporated when they are next published. |
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 2m 58s (non-voting) |
...things that are not touched by this PR. |
recheck |
Build succeeded. ❌ ansible-galaxy-importer FAILURE in 4m 31s (non-voting) |
Build succeeded. ❌ ansible-galaxy-importer FAILURE in 5m 12s (non-voting) |
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 3m 24s (non-voting) |
@@ -177,11 +406,30 @@ def describe_elb(connection, lb): | |||
|
|||
@AWSRetry.jittered_backoff() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tremble @mandar242 @markuman What do you think about moving some of these utilities to a module_utils folder in community.aws and calling it probably module_utils/elb.py or module_utils/elb_utils.py (I have a proposal to rename the actual elb_utils.py to elbv2_utils.py ansible-collections/amazon.aws#2285)? This will give a good start that can be further explored once the module and utility are migrated to amazon.aws and will ease the testing process, since we do not need cross-repo testing for the time being. I would like us to follow a similar pattern to what we have in amazon.aws.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I'd do the move after we've moved the code to amazon.aws, cleaning up both the elb_classic_lb and elb_classic_lb_info modules at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alinabuzachis , I agree with @tremble, once elb_classic_lb_info.py
is moved to amazon.aws
then module_utils for both elb_classic_lb and elb_classic_lb_info
can be refactored in a better way in my opinion.
Build succeeded. ❌ ansible-galaxy-importer FAILURE in 5m 23s (non-voting) |
LGTM - Just make sure to address the linter failures. |
milestone and devel failing sanity on things untouched by this PR
|
5ae927d
to
ba9cd61
Compare
Build succeeded (gate pipeline). ❌ ansible-galaxy-importer FAILURE in 5m 04s (non-voting) |
2ad8a8f
into
ansible-collections:main
…lections#2139) SUMMARY Added type hints and function descriptions. Updated return block of the module. ISSUE TYPE Docs Pull Request COMPONENT NAME elb_classic_lb_info ADDITIONAL INFORMATION Reviewed-by: Markus Bergholz <[email protected]> Reviewed-by: Alina Buzachis Reviewed-by: Mandar Kulkarni <[email protected]> Reviewed-by: Mark Chappell Reviewed-by: GomathiselviS
SUMMARY
Added type hints and function descriptions.
Updated return block of the module.
ISSUE TYPE
COMPONENT NAME
elb_classic_lb_info
ADDITIONAL INFORMATION