-
Notifications
You must be signed in to change notification settings - Fork 4.6k
[python] Add Secret management module in apache_beam.utils.secret #39636
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
base: master
Are you sure you want to change the base?
Changes from all commits
1d40eea
59da141
3c3dfa7
881ba7f
c9ccb6c
29c2f5a
59ce11b
5aebe64
e9a7666
8446361
fe3870a
4d5b1d9
5af797f
7c9e976
4ee7e8b
12f424e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -243,7 +243,7 @@ def test_co_group_by_key_on_unpickled(self): | |
| assert_that(pcoll, equal_to(expected)) | ||
|
|
||
|
|
||
| class FakeSecret(beam.Secret): | ||
| class FakeSecret(beam.utils.secret.Secret): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do all of these tests still belong in this file? Should they be moved if we're doing a refactor into a new module?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will refactor the secret related to the new test file of the module, but GBEK tests will stay in util_test.py. |
||
| def __init__(self, version_name=None, should_throw=False): | ||
| self._secret = b'aKwI2PmqYFt2p5tNKCyBS5qYmHhHsGZcyZrnZQiQ-uE=' | ||
| self._should_throw = should_throw | ||
|
|
@@ -308,7 +308,8 @@ def test_secret_manager_parses_correctly(self, secret_string, secret): | |
| exception_str='must contain a valid type parameter'), | ||
| param( | ||
| secret_string='type:gcpsecreT', | ||
| exception_str='missing 1 required positional argument'), | ||
| exception_str= | ||
| r"Secret name \('name'\) must be specified in secret spec."), | ||
| param( | ||
| secret_string='type:gcpsecreT;version_name:foo;extra:val', | ||
| exception_str='Invalid secret parameter extra'), | ||
|
|
@@ -387,7 +388,9 @@ def test_gbek_fake_secret_manager_actually_does_encryption(self): | |
| result, equal_to([('a', ([1, 2])), ('b', ([3])), ('c', ([4]))])) | ||
|
|
||
| @mock.patch('apache_beam.transforms.util._DecryptMessage', MockNoOpDecrypt) | ||
| @mock.patch('apache_beam.transforms.util.GcpSecret', FakeSecret) | ||
| @mock.patch.dict( | ||
| 'apache_beam.utils.secret._SECRET_CLASSES', | ||
| {'googlecloudsecretmanager': FakeSecret}) | ||
| def test_gbk_actually_does_encryption(self): | ||
| options = PipelineOptions() | ||
| # Version of GcpSecret doesn't matter since it is replaced by FakeSecret | ||
|
|
||
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.
This PR mixes a bunch of non-functional changes (moving from one directory to another while retaining imports, renaming methods) and some smaller targeted functional changes. This makes it difficult to review because its hard to identify what actually changed, and if it does introduce issues it will be harder to track them down as a result. Could we split this change into 2 changes, one to move things into the utils directory and one to make any additional changes to the classes themselves? This could still be 2 commits in the same PR or it could be 2 PRs.
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.
True. Let me split it into two commits then.