-
Notifications
You must be signed in to change notification settings - Fork 342
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
Code refactoring - module_utils/elbv2 #2050
Code refactoring - module_utils/elbv2 #2050
Conversation
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 3m 51s |
9eaf4b2
to
13be34e
Compare
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 5m 01s |
recheck |
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 5m 39s |
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 6m 08s |
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.
I think this is a great step towards making these modules easier to understand! I made a few comments inline and I think there is probably more we can do to clarify the rules comparison in the future, but in general this looks good to me.
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 5m 14s |
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.
Just a few more minor comments.
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 5m 56s |
self.fail_json(msg="Modifying subnets and elastic IPs is not supported for Network Load Balancer") | ||
|
||
|
||
def _compare_listener(current_listener: Dict[str, Any], new_listener: Dict[str, Any]) -> Optional[Dict[str, Any]]: |
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.
I would probably suggest to split this function into several ones.
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.
This function has several lines, but overall what is done inside is small enough to remain in this function
|
||
|
||
def _group_rules( | ||
current_rules: List[Dict[str, Any]], rules: List[Dict[str, Any]] |
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.
What would you think about splitting this one too?
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.
same here, this is a small function
Build failed (gate pipeline). For information on how to proceed, see https://ansible.softwarefactory-project.io/zuul/buildset/4c8b2eb7c5ef4e818b704f7a5e3c0364
|
recheck |
regate |
Build failed (gate pipeline). For information on how to proceed, see https://ansible.softwarefactory-project.io/zuul/buildset/142c35415688497db7a77854308a9acb ✔️ ansible-galaxy-importer SUCCESS in 5m 23s |
regate |
Build failed.
|
recheck |
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 5m 48s |
Build succeeded (gate pipeline). ✔️ ansible-galaxy-importer SUCCESS in 4m 01s |
993cbd1
into
ansible-collections:main
Code refactoring - module_utils/elbv2 SUMMARY closes ansible-collections#1981 ISSUE TYPE Feature Pull Request Reviewed-by: Helen Bailey <[email protected]> Reviewed-by: Bikouo Aubin Reviewed-by: Alina Buzachis
SUMMARY
closes #1981
ISSUE TYPE