-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Add validation for SecretsManagerKmsKeyId #2323
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
Conversation
tests/translator/input/error_function_with_mq_kms_invalid_type.yaml
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## develop #2323 +/- ##
===========================================
+ Coverage 93.58% 94.29% +0.71%
===========================================
Files 90 97 +7
Lines 6124 7116 +992
Branches 1260 1466 +206
===========================================
+ Hits 5731 6710 +979
- Misses 183 196 +13
Partials 210 210
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.
Can we add unit tests as well? The end to end tests are great but we can create a better coverage if we add unit tests and validate different input (lists, dict, int, etc).
def test_must_validate_secrets_manager_kms_key_id(self): | ||
self.mq_event_source.SourceAccessConfigurations = [{"Type": "BASIC_AUTH", "URI": "SECRET_URI"}] | ||
self.mq_event_source.Broker = "BROKER_ARN" | ||
self.mq_event_source.SecretsManagerKmsKeyId = ["1abc23d4-567f-8ab9-cde0-1fab234c5d67"] |
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.
Could we parameterize this to include anything that is not a string? We covered list here but would be good to add int, bool and dict to add move coverage.
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.
Thank you for going through additional iterations with adding more unit tests.
Description of changes:
Added validation to SecretsManagerKmsKeyId
Description of how you validated changes:
validation will ensure that SecretsManagerKmsKeyId is of type string
Checklist:
make pr
passesExamples?
Please reach out in the comments, if you want to add an example. Examples will be
added to
sam init
through https://github.com/awslabs/aws-sam-cli-app-templates/By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.