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

elb_classic_lb_info: Refactor elb_classic_lb_info module #2139

Conversation

mandar242
Copy link
Contributor

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

Copy link

github-actions bot commented Sep 4, 2024

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

Copy link
Contributor

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/ad051fbc29bb4b739f166502c72ac2e8

✔️ ansible-galaxy-importer SUCCESS in 2m 58s (non-voting)
✔️ build-ansible-collection SUCCESS in 10m 32s
✔️ ansible-test-splitter SUCCESS in 4m 17s
✔️ integration-community.aws-1 SUCCESS in 13m 24s
✔️ integration-community.aws-2 SUCCESS in 5m 02s
Skipped 20 jobs

@mandar242 mandar242 changed the title [WIP] elb_classic_lb_info: Refactor elb_classic_lb_info module elb_classic_lb_info: Refactor elb_classic_lb_info module Sep 5, 2024
@markuman
Copy link
Member

markuman commented Sep 5, 2024

ERROR: Found 5 pylint issue(s) which need to be resolved:
ERROR: plugins/modules/ec2_vpc_vgw.py:504:13: used-before-assignment: Using variable 'deleted_vgw' before assignment
ERROR: plugins/modules/ecs_cluster.py:342:12: unreachable: Unreachable code
ERROR: plugins/modules/ecs_cluster.py:364:12: unreachable: Unreachable code
ERROR: plugins/modules/ecs_service.py:1251:12: unreachable: Unreachable code
ERROR: plugins/modules/ecs_service.py:1266:12: unreachable: Unreachable code

...things that are not touched by this PR.

@markuman
Copy link
Member

markuman commented Sep 5, 2024

recheck

Copy link
Contributor

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/2206868602384bd6b087b3544a2a21a3

ansible-galaxy-importer FAILURE in 4m 31s (non-voting)
✔️ build-ansible-collection SUCCESS in 11m 34s
✔️ ansible-test-splitter SUCCESS in 4m 19s
✔️ integration-community.aws-1 SUCCESS in 14m 41s
✔️ integration-community.aws-2 SUCCESS in 7m 10s
Skipped 20 jobs

plugins/modules/elb_classic_lb_info.py Outdated Show resolved Hide resolved
plugins/modules/elb_classic_lb_info.py Outdated Show resolved Hide resolved
plugins/modules/elb_classic_lb_info.py Show resolved Hide resolved
plugins/modules/elb_classic_lb_info.py Show resolved Hide resolved
plugins/modules/elb_classic_lb_info.py Show resolved Hide resolved
Copy link
Contributor

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/cbf36a484f864eacad49fa8ae73ae092

ansible-galaxy-importer FAILURE in 5m 12s (non-voting)
✔️ build-ansible-collection SUCCESS in 10m 36s
✔️ ansible-test-splitter SUCCESS in 4m 25s
✔️ integration-community.aws-1 SUCCESS in 13m 10s
✔️ integration-community.aws-2 SUCCESS in 5m 12s
Skipped 20 jobs

Copy link
Contributor

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/8c8cb8855d094137a3740e78b4dda90e

✔️ ansible-galaxy-importer SUCCESS in 3m 24s (non-voting)
✔️ build-ansible-collection SUCCESS in 10m 34s
✔️ ansible-test-splitter SUCCESS in 4m 21s
✔️ integration-community.aws-1 SUCCESS in 13m 22s
✔️ integration-community.aws-2 SUCCESS in 5m 11s
Skipped 20 jobs

plugins/modules/elb_classic_lb_info.py Outdated Show resolved Hide resolved
plugins/modules/elb_classic_lb_info.py Show resolved Hide resolved
plugins/modules/elb_classic_lb_info.py Outdated Show resolved Hide resolved
plugins/modules/elb_classic_lb_info.py Outdated Show resolved Hide resolved
plugins/modules/elb_classic_lb_info.py Outdated Show resolved Hide resolved
plugins/modules/elb_classic_lb_info.py Outdated Show resolved Hide resolved
plugins/modules/elb_classic_lb_info.py Outdated Show resolved Hide resolved
@@ -177,11 +406,30 @@ def describe_elb(connection, lb):

@AWSRetry.jittered_backoff()
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/e298f5973b9345e489a624cc7570fb8f

ansible-galaxy-importer FAILURE in 5m 23s (non-voting)
✔️ build-ansible-collection SUCCESS in 10m 42s
✔️ ansible-test-splitter SUCCESS in 4m 15s
✔️ integration-community.aws-1 SUCCESS in 13m 52s
✔️ integration-community.aws-2 SUCCESS in 7m 25s
Skipped 20 jobs

@GomathiselviS
Copy link
Contributor

LGTM - Just make sure to address the linter failures.

@mandar242
Copy link
Contributor Author

milestone and devel failing sanity on things untouched by this PR

ERROR: Found 7 pylint issue(s) which need to be resolved:
ERROR: plugins/modules/ec2_vpc_vgw.py:504:13: used-before-assignment: Using variable 'deleted_vgw' before assignment
ERROR: plugins/modules/ecs_cluster.py:294:16: collection-deprecated-version: Deprecated version ('9.0.0') found in call to Display.deprecated or AnsibleModule.deprecate
ERROR: plugins/modules/ecs_cluster.py:342:12: unreachable: Unreachable code
ERROR: plugins/modules/ecs_cluster.py:364:12: unreachable: Unreachable code
ERROR: plugins/modules/ecs_service.py:1251:12: unreachable: Unreachable code
ERROR: plugins/modules/ecs_service.py:1266:12: unreachable: Unreachable code
ERROR: plugins/modules/glue_connection.py:335:8: collection-deprecated-version: Deprecated version ('9.0.0') found in call to Display.deprecated or AnsibleModule.deprecate

@mandar242 mandar242 added the mergeit Merge the PR (SoftwareFactory) label Sep 26, 2024
@mandar242 mandar242 added mergeit Merge the PR (SoftwareFactory) and removed mergeit Merge the PR (SoftwareFactory) labels Sep 26, 2024
Copy link
Contributor

Build succeeded (gate pipeline).
https://ansible.softwarefactory-project.io/zuul/buildset/bd2a25637d754953a1653e91b4f31117

ansible-galaxy-importer FAILURE in 5m 04s (non-voting)
✔️ build-ansible-collection SUCCESS in 10m 51s
✔️ ansible-test-splitter SUCCESS in 4m 18s
✔️ integration-community.aws-1 SUCCESS in 14m 54s
✔️ integration-community.aws-2 SUCCESS in 6m 59s
Skipped 20 jobs

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 2ad8a8f into ansible-collections:main Sep 26, 2024
66 of 96 checks passed
alinabuzachis pushed a commit to GomathiselviS/community.aws that referenced this pull request Oct 1, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mergeit Merge the PR (SoftwareFactory)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants