Issue135 (Cannot add key to Encryption Context if already there in default CMM) #185
Conversation
This file contains 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
added 4 commits
August 5, 2019 18:28
…all changes so formatting may be slightly different, but it should be the same functionality wise. I have a couple errors that I believe are not from my own changes (they were there on master when I pulled and pertain to there not being valid AWS credentials for tests for the most part), but they should not be important.
instead of AlgorithmSuite. Changed messages in the docs accordingly. Now if you go to RTD you will see everything links together nicely. What I did not do: change the variable names from "algorithm" to i.e. "algorithm_suite". Mainly because this is a rabbit hole and not immediately important I think. To do it you need to change every single thing that calls another thing that calls another thing that has "algorithm" instead of "algorithm_suite" and I wanted to keep it simple. I also found what I think was an incorrect test. In /test/integration/test_i_aws_encryption_sdk_client.py in function test_remove_bad_client() there was an assertion error for my tox env tests that the dict containing the regional clients was not empty when it should have been supposedly. I believe that it was not meant for it to be empty, but for the bad client "us-fakey-12" to be removed, and it was assumed that in __init__ for KMSMasterKeyProvider() no regional clients were added, but this is false because there is a default added depending on some features of the botocore session etc... So when it was checking to see if it was empty it wanted to see that a bad client was added, and then was rightfully removed -> back to an empty dict, but if the dict did not start out empty the test would fail. So I made a one line change of this test to make it test that specifically the dict did not contain the exact bad client that the test was using. Please get back to me on this final change because you guys know this SDK way better than me and I don't want to break anything. Thanks!
get the value of the key in the encryption context dictionary to be the same (as well as name of the key), but then learned this wasn't what we wanted. Wrote a test that I confirm works only after the change is added.
4gatepylon
changed the title
There is some overlap because it split from master at the same time as the other issue (basically I fixed the other issue in master and then decided it would be cleaner to put it in its own branch but didn't hard reset before splitting and editing in this branch, so it carries over some commits. Resetting now would delete the additional commits that deal with issue 135. This is my mistake and I can handle it manually if necessary but the functionality should not be changed if you merge this like nothing happened.) PR #187 fixes a sphinx linking issue for our docs. This PR (does that and also) fixes an issue where keys are already present in an encryption context. |
|
Closing stale PR, please reopen as needed |
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.
Deals with Issue #135
Added a test to see whether it was possible to add another key with the same name or not. The expected correct behavior is that an error will be thrown in this case (for more context read he issue). Then fixed the issue and it works. It ignores the value of the key, and only looks at the name.
Please only look at the most recent commit fixing issue135. The others are left over from when I branched in my fork after fixing a Sphinx issue.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable:
No files moved, deleted, or added, but they were edited.