kms_key: Add multi region support to create_key #1290
Conversation
Signed-off-by: GomathiselviS <[email protected]>
Docs Build 📝Thank you for contribution!✨ This PR has been merged and your docs changes will be incorporated when they are next published. |
ansibullbot
added
community_review
feature
|
|
||
|
|
||
| module_name = 'ansible_collections.amazon.aws.plugins.modules.kms_key' | ||
| key_details = {'KeyMetadata': |
There was a problem hiding this comment.
It's a new file, may I suggest to use Black to indent it.
There was a problem hiding this comment.
Tests pass for me locally with a multi-region capable AWS account, with the noted change in the test file. LGTM otherwise.
Normally I'd prefer to keep sweeping formatting changes in their own PRs, this is a large diff for what's actually a fairly small code change, but I won't block this PR for that.
tests/integration/targets/kms_key/roles/aws_kms/tasks/test_multi_region.yml
Show resolved
Hide resolved
Using 'black' to format the file resulted in all those changes. |
plugins/modules/kms_key.py
Outdated
| from ansible_collections.amazon.aws.plugins.module_utils.ec2 import ( | ||
| ansible_dict_to_boto3_tag_list, | ||
| ) | ||
| from ansible_collections.amazon.aws.plugins.module_utils.ec2 import ( | ||
| boto3_tag_list_to_ansible_dict, | ||
| ) | ||
| from ansible_collections.amazon.aws.plugins.module_utils.ec2 import ( | ||
| camel_dict_to_snake_dict, | ||
| ) |
There was a problem hiding this comment.
These line breaks really don't add anything, please can we at least use a longer line length for black
There was a problem hiding this comment.
Was there an internal decision that you're moving to using black? Honestly some of these blind changes make things less readable rather than more.
There was a problem hiding this comment.
Not a formal decision for Python content, no. We will be using ansible-lint for user-facing YAML content like roles though so there's some motivation to try to be more consistent in formatting generally.
There was a problem hiding this comment.
We mentioned that it's an option. I didn't think anything had been agreed upon. We certainly should look into what line length we use, the ~100 seems to be too short and results in things like the above.
There was a problem hiding this comment.
That's right, it wasn't something agreed upon, it was only a suggestion due to the massive number of PR adding unit tests and lots of refactoring for module_utils. However, I guess we should come up with a strategy first before applying consistent formatting.
There was a problem hiding this comment.
For this PR I am planning to undo the formatting done by black for the already existing file - plugins/modules/kms_key.py . I shall format the newly added unit test file with black -l 160 <file> (line length = 160). @tremble @jillr @alinabuzachis What is your view on that?
…1290) kms_key: Add multi region support to create_key Signed-off-by: GomathiselviS [email protected] SUMMARY Fixes ansible-collections#1281 ISSUE TYPE Feature Pull Request COMPONENT NAME ADDITIONAL INFORMATION Reviewed-by: Gonéri Le Bouder <[email protected]> Reviewed-by: Jill R <None> Reviewed-by: Mark Chappell <None> Reviewed-by: Alina Buzachis <None> Reviewed-by: GomathiselviS <None>
[manual backport stable-5] kms_key: Add multi region support to create_key (#1290) kms_key: Add multi region support to create_key Signed-off-by: GomathiselviS [email protected] SUMMARY Fixes #1281 ISSUE TYPE Feature Pull Request COMPONENT NAME ADDITIONAL INFORMATION Reviewed-by: Gonéri Le Bouder [email protected] Reviewed-by: Jill R Reviewed-by: Mark Chappell Reviewed-by: Alina Buzachis Reviewed-by: GomathiselviS 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]
SUMMARY
Fixes #1281
ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION