-
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
Add metrics and extended_statistic keys to cloudwatch module #1133
Add metrics and extended_statistic keys to cloudwatch module #1133
Conversation
Docs Build 📝Thank you for contribution!✨ This PR has been merged and your docs changes will be incorporated when they are next published. |
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.
A few small comments. Also, I just realized that this module has no documentation for RETURN values. Is that intentional?
if module.params.get('extended_statistic'): | ||
params['ExtendedStatistic'] = module.params.get('extended_statistic') | ||
|
||
for key, value in list(params.items()): |
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.
Why use list()
here?
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.
@gravesm list() is used here because it preserves the keys & values as they appeared when the list was created. If not, the usage of del
inside the loop will result in RuntimeError: dictionary changed size during iteration
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.
Ah, right. You could use a dictionary comprehension here which would be a little more pythonic, but either way is fine.
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.
Why are you deleting None values? I guess it would be easier to set a params only if defined instead. Otherwise, you have to cover that functionality with a unit test.
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 The keys that are not defined in the task will be set to 'None' value. I am deleting the None values in order to satisfy the mutually exclusive condition. I will add a test case to test the mutually exclusive condition to the integration test suite, for now.
Pull request merge failed: Resource not accessible by integration, You may need to manually rebase your PR and retry. |
Signed-off-by: GomathiselviS <[email protected]>
7e25263
to
fe01d57
Compare
…-collections#1133) Add metrics and extended_statistic keys to cloudwatch module Signed-off-by: GomathiselviS [email protected] To support https://issues.redhat.com/browse/ACA-638 , a new key metric ( a list of dicts) is added to the cloudwatch module SUMMARY ISSUE TYPE Feature Pull Request COMPONENT NAME cloudwatch.py ADDITIONAL INFORMATION Reviewed-by: Bikouo Aubin <None> Reviewed-by: Gonéri Le Bouder <[email protected]> Reviewed-by: Mike Graves <[email protected]> Reviewed-by: GomathiselviS <None> Reviewed-by: Alina Buzachis <None>
…-collections#1133) Add metrics and extended_statistic keys to cloudwatch module Signed-off-by: GomathiselviS [email protected] To support https://issues.redhat.com/browse/ACA-638 , a new key metric ( a list of dicts) is added to the cloudwatch module SUMMARY ISSUE TYPE Feature Pull Request COMPONENT NAME cloudwatch.py ADDITIONAL INFORMATION Reviewed-by: Bikouo Aubin <None> Reviewed-by: Gonéri Le Bouder <[email protected]> Reviewed-by: Mike Graves <[email protected]> Reviewed-by: GomathiselviS <None> Reviewed-by: Alina Buzachis <None>
…1485) [manual backport stable-5] Add metrics and extended_statistic keys to cloudwatch module (#1133) Add metrics and extended_statistic keys to cloudwatch module Signed-off-by: GomathiselviS [email protected] To support https://issues.redhat.com/browse/ACA-638 , a new key metric ( a list of dicts) is added to the cloudwatch module SUMMARY ISSUE TYPE Feature Pull Request COMPONENT NAME cloudwatch.py ADDITIONAL INFORMATION Reviewed-by: Bikouo Aubin Reviewed-by: Gonéri Le Bouder [email protected] Reviewed-by: Mike Graves [email protected] Reviewed-by: GomathiselviS Reviewed-by: Alina Buzachis SUMMARY ISSUE TYPE Bugfix Pull Request Docs Pull Request Feature Pull Request New Module Pull Request COMPONENT NAME ADDITIONAL INFORMATION
Signed-off-by: GomathiselviS [email protected]
To support https://issues.redhat.com/browse/ACA-638 , a new key metric ( a list of dicts) is added to the cloudwatch module
SUMMARY
ISSUE TYPE
COMPONENT NAME
cloudwatch.py
ADDITIONAL INFORMATION