Fixed KMS master key provider tests when default AWS region is configured #179
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…AWS region configured
mattsb42-aws
requested changes
Jul 29, 2019
ragona
commented
Jul 29, 2019
|
Thanks for the very helpful comments, @mattsb42-aws! I think I've responded to all of your comments with a much-improved way to fix this that doesn't involve skipping any tests. I'm not sure if I followed the general best practices and conventions for creating that fixture though, so definitely looking for feedback on general style. |
mattsb42-aws
reviewed
Aug 2, 2019
There was a problem hiding this comment.
I like this, but there's one thing I want to add to it before merging.
I want to make sure that we are testing everything in this module both with a default region set and not set to make sure that we do not regress.
For simplicity, I'm just going to push a commit to your branch, because this gets a bit into dark pytest magics.
…gainst both with and without default region
mattsb42-aws
approved these changes
Aug 2, 2019
There was a problem hiding this comment.
Assuming Travis passes, this LGTM.
@ragona before merging, please take a look at the fixture I added and make sure it makes sense to you. If not, we'll expand docs/etc before merging.
mattsb42-aws
added a commit
that referenced
this pull request
Oct 4, 2019
* Update PR template * Added a check for max_age being greater than 0 (#172) * Added a check for max_age being greater than 0 * Fixed flake8 by adding missing pydocstyle dependency * Added the dependency to decrypt_oracle as well * Added test for max_age<=0 ValueError * Updated test for max_age<=0.0 ValueError * Added negative test case * Fixed KMS master key provider tests when default AWS region is configured (#179) * Fixed KMS master key provider tests for users who have their default AWS region configured * created fixture for botocore session with no region set * add auto-used fixture in KMS master key provider unit tests to test against both with and without default region * Wrote example and test for using one kms cmk with an unsigned algorithm * Update one_kms_cmk_unsigned.py * Update examples/src/one_kms_cmk_unsigned.py Co-Authored-By: Matt Bullock <[email protected]> * isort-check now succeeds * [issue-190] Regional clients modify default botocore session (#193) * [issue-190] Creation of regional clients modifies default botocore session's region * update changelog with changes for 1.4.1 release * bump version to 1.4.1 * Updates to handle new pylint requirements (#196) * pylint max-attributes appears to be ratcheted down recently * remove unnecessary comprehensions * whitelist some pylint use-constant-test false-positives * reorganize backwards compatibility test requirements definitions attrs==19.2.0 removed a deprecated feature that aws-encryption-sdk==1.3.3 depended on. This reorganization lets us define specific requirements bounds for old versions of aws-encryption-sdk that will probably continue to be necessary as these old versions age. * remove unnecessary comprehensions * add newlines to the end of all requirements files * help pylint ignore mypy type use
mattsb42-aws
added a commit
that referenced
this pull request
Mar 12, 2020
* Update PR template * Added a check for max_age being greater than 0 (#172) * Added a check for max_age being greater than 0 * Fixed flake8 by adding missing pydocstyle dependency * Added the dependency to decrypt_oracle as well * Added test for max_age<=0 ValueError * Updated test for max_age<=0.0 ValueError * Added negative test case * Testing something, want AppVeyor to run * Quick change * Running AppVeyor * Added example for using multiple keyrings in multiple regions * Undid something quickly * Fixed importerror * Formatting fix * Update tox.ini * Update tox.ini * Made some changes to the multiple_kms_cmk_regions example/test * This is my next interation of the code for the example; however, I am still working on populating the tests correctly, so the CI will fail, but I tested the code with my own KMS CMK ARNs, so I know it will work once the tests are populated (working with Tejeswini on this) * Changed the example to test two CMKs in the same region until Issue #178 is cleared up * Found out how to make a new valid test key, so now there are two valid test keys in different regions for this example * Ran autoformat * Added some docstrings * Formatting will be the death of me * Used correct keys in test * Updated some comments * Fixed KMS master key provider tests when default AWS region is configured (#179) * Fixed KMS master key provider tests for users who have their default AWS region configured * created fixture for botocore session with no region set * add auto-used fixture in KMS master key provider unit tests to test against both with and without default region * Wrote example and test for using one kms cmk with an unsigned algorithm * Update the integration tests * Small changes * Update one_kms_cmk_unsigned.py * Update examples/src/one_kms_cmk_unsigned.py Co-Authored-By: Matt Bullock <[email protected]> * isort-check now succeeds * chore: move existing examples into "legacy" directory * chore: add automatic test runner for examples * chore: convert existing examples to work with automatic test runner * chore: move examples kwarg building into utils module * chore: convert multi-CMK test runners to new configuration format * fix: fix multi-CMK example logic * chore: convert multi-CMK example to new test runner signature and move into legacy examples * chore: make examples linting always run across both source and tests * fix: linting fixes * docs: add docstring description for legacy examples module * chore: add initial new-format examples * docs: add examples readme * docs: add instructions for writing examples * chore: address PR comments * chore: change examples parameter from aws_kms_cmk_arn to aws_kms_cmk for consistency * docs: clarify integration tests readme * Apply suggestions from code review Co-Authored-By: June Blender <[email protected]> * Apply suggestions from code review Co-Authored-By: June Blender <[email protected]> * docs: change from "one-shot" term to "one-step" * chore: apply changes based on PR comments * docs: reword parameter description * Apply suggestions from code review Co-Authored-By: June Blender <[email protected]> * chore: rename examples input parameters to move from "child" to "additional" naming * docs: clarify configuration intro * docs: apply examples comments consistently * chore: fix line length * fix: fix typo Co-authored-by: John Walker <[email protected]> Co-authored-by: Caitlin Tibbetts <[email protected]> Co-authored-by: Caitlin Tibbetts <[email protected]> Co-authored-by: Ryan Ragona <[email protected]> Co-authored-by: lizroth <[email protected]> Co-authored-by: June Blender <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes issue #31, in which users who have configured a default AWS region (either via ~/.aws/config or via environment variable) will experience failing tests for
KMSMasterKeyProvider. I tried to keep the tests mostly unchanged, but in some cases I elected to simply skip them since the test didn't provide much value if there was already a default value configured.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.