-
Notifications
You must be signed in to change notification settings - Fork 341
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
Refactor inventory plugins #1218
Refactor inventory plugins #1218
Conversation
12c06d6
to
7bb6583
Compare
I was terrified when I first saw the size of the PR, but it looks pretty cool. I'm not a super huge fan of multi-inheritance because it's often annoying to troubleshoot. I would prefer some composition instead. For instance, we can store an instance of |
1870f86
to
a5309df
Compare
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 not a bit fan of how some of this credential/region-discovery logic is being handled, however changing it is probably worth a separate PR.
I'd like @goneri's input on the remaining unit tests, but I think this is otherwise good enough for now.
plugins/plugin_utils/inventory.py
Outdated
if iam_role_arn is not None: | ||
try: | ||
# Describe regions assuming arn role | ||
assumed_credentials = self._boto3_assume_role(credentials, resource) |
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.
There's a lot of separate assume calls going on here, we only need to assume the role once, this should be pulled up to line 114.
These excessive assume calls are probably the root cause of #1091
plugins/plugin_utils/inventory.py
Outdated
if not regions: | ||
try: |
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.
If we're explicitly passed iam_assume_role, it kinda feels like we should be trying the assumed role first, and falling back as an act of last resort.
recheck |
recheck |
recheck |
add unit test for aws_rds inventory reduce time and support aws_rds integration tests
80f56b4
to
c78aa83
Compare
…sible-collections#1218) Tagging - wafv2_web_acl add support for managing and purging tags SUMMARY Add support for returning tags Add support for updating tags Add support for purge_tags ISSUE TYPE Feature Pull Request COMPONENT NAME wafv2_web_acl wafv2_web_acl_info ADDITIONAL INFORMATION Reviewed-by: Joseph Torcasso <None> Reviewed-by: Mark Chappell <None> Reviewed-by: Alina Buzachis <None>
…sible-collections#1218) Tagging - wafv2_web_acl add support for managing and purging tags SUMMARY Add support for returning tags Add support for updating tags Add support for purge_tags ISSUE TYPE Feature Pull Request COMPONENT NAME wafv2_web_acl wafv2_web_acl_info ADDITIONAL INFORMATION Reviewed-by: Joseph Torcasso <None> Reviewed-by: Mark Chappell <None> Reviewed-by: Alina Buzachis <None>
…sible-collections#1218) Tagging - wafv2_web_acl add support for managing and purging tags SUMMARY Add support for returning tags Add support for updating tags Add support for purge_tags ISSUE TYPE Feature Pull Request COMPONENT NAME wafv2_web_acl wafv2_web_acl_info ADDITIONAL INFORMATION Reviewed-by: Joseph Torcasso <None> Reviewed-by: Mark Chappell <None> Reviewed-by: Alina Buzachis <None>
SUMMARY
ISSUE TYPE
COMPONENT NAME
aws_rds
aws_ec2