Fixed issue #61 #187
Closed
Fixed issue #61 #187
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
added 3 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!
ragona
suggested changes
Aug 13, 2019
| AES_128_GCM_IV12_TAG16_NO_PADDING = (EncryptionType.SYMMETRIC, Algorithm.AES_128_GCM_IV12_TAG16, None, None, None) | ||
| AES_192_GCM_IV12_TAG16_NO_PADDING = (EncryptionType.SYMMETRIC, Algorithm.AES_192_GCM_IV12_TAG16, None, None, None) | ||
| AES_256_GCM_IV12_TAG16_NO_PADDING = (EncryptionType.SYMMETRIC, Algorithm.AES_256_GCM_IV12_TAG16, None, None, None) | ||
| AES_128_GCM_IV12_TAG16_NO_PADDING = ( |
There was a problem hiding this comment.
Any specific reason you changed the formatting style here? (Many lines vs. one line per constant?) These should all match.
There was a problem hiding this comment.
this is a function of tox -re autoformat (and python black).
There was a problem hiding this comment.
Auto-formatting does this because some lines are too long, but some aren't. Making them all one line will fail flake8. They do pass if you make them all the new format, however. So the options are either to add some variables to shorten the text and make them all one line or make them all the new format. I think the latter is more readable, but both are valid options. I've committed changes to standardize to the second option tentatively.
|
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.
Same as PR #182 but actually works. Thank goodness for git reset --hard because otherwise the keyring stuff just messed up my entire PR.
This replaces all references to Algorithm with references to AlgorithmSuite to fix readthedocs Sphinx linking.