-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Refactoring/aws/elbv2 #259
Conversation
Codecov Report
@@ Coverage Diff @@
## refactoring/resource-configs #259 +/- ##
===============================================================
+ Coverage 30.72% 30.9% +0.18%
===============================================================
Files 126 130 +4
Lines 5601 5633 +32
===============================================================
+ Hits 1721 1741 +20
- Misses 3880 3892 +12
Continue to review full report at Codecov.
|
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 apologize for the troubles this has caused you, stay strong! 💪
@@ -112,6 +111,18 @@ def _add_security_group_name_to_ec2_grants(self): | |||
{'AWSAccountId': self.aws_account_id}) | |||
|
|||
def _add_security_group_data_to_elbv2(self): | |||
def check_security_group_rules(lb, index, traffic_type): |
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.
Wait what
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'm so confused what is this
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.
It's just a function that Vincent defined for his findings related to ELBv2, is only used by _add_security_group_data_to_elbv2
and was in aws/services/elbv2.py
. So it wasn't really make sense to move it in aws/resources/elbv2/
directory, moving it to _add_security_group_data_to_elbv2
as a nested function where it is used makes more sense imho (at least now, waiting for a refactoring of all the post-processing logic).
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'm confused by the nested method though. But great work 💯
This PR aims to migrate AWS ELBv2 service to the new architecture, as part of #183.
Please note that I removed the
ssl_policies
resources (defined in themetadata.json
file) since they weren't used anywhere.