[python] Add Secret management module in apache_beam.utils.secret - #39636
[python] Add Secret management module in apache_beam.utils.secret#39636shunping wants to merge 4 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
r: @damccorm |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
95b1d10 to
4d5b1d9
Compare
| For example, 'type:GcpSecret;version_name:my_secret/versions/latest' | ||
| would return a GcpSecret initialized with 'my_secret/versions/latest'. | ||
|
|
||
| Deprecated: Use :meth:`from_option_string` instead. |
There was a problem hiding this comment.
Any reason to deprecate this? Is it just a naming change?
There was a problem hiding this comment.
Yes, it is a name change for consistency with the new function from_spec(), since it is already released, I think using a deprecate path is better than directly changing that and causing breaking change immediately. WDYT?
There was a problem hiding this comment.
Is there a reason for the name change?
There was a problem hiding this comment.
I think when it comes to a constructor, from_option_string or from_option is more pythonic than parse_secret_option. Also it is consistent with the new from_spec function.
There was a problem hiding this comment.
I think when it comes to a constructor, from_option_string or from_option is more pythonic than parse_secret_option
This is probably true, but that also probably doesn't make it worth taking on the extra cost of having a deprecated method (that we'll either have forever or will eventually remove in a breaking change).
Also it is consistent with the new from_spec function.
I think we might want to avoid this naming anyways because it is similar to
but not actually referencing the same kind of spec. But it also isn't clear to me why we need this anywaysThere was a problem hiding this comment.
We can put that logic of parsing json representation inside parse_secret_option, but I think it is clearer to have a separate function.
If from_spec is not a good name, maybe we can use from_json?
There was a problem hiding this comment.
Should it be a public method? Maybe _from_json?
There was a problem hiding this comment.
It should be public. We will use it to parse the password json string in jdbcio and others.
|
|
||
|
|
||
| class FakeSecret(beam.Secret): | ||
| class FakeSecret(beam.utils.secret.Secret): |
There was a problem hiding this comment.
Do all of these tests still belong in this file? Should they be moved if we're doing a refactor into a new module?
There was a problem hiding this comment.
I will refactor the secret related to the new test file of the module, but GBEK tests will stay in util_test.py.
There was a problem hiding this comment.
FakeSecret is actually staying in util_test.py because it is only used by GBEK tests.
| return pcoll | 'RemoveDuplicates' >> Distinct() | ||
|
|
||
|
|
||
| class Secret(): |
There was a problem hiding this comment.
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.
True. Let me split it into two commits then.
There is no functional changes in this commit. We also re-export the secret classes in apache_beam.transforms.util for backward compatibility.
12f424e to
89781fd
Compare
2ba341f to
45bb552
Compare
3 participants
Description
Introduces
apache_beam.utils.secretto centralize secret management in the Python SDK.Changes:
Secretbase class and implementations (RawSecret,GcpSecret, andGcpHsmGeneratedSecret) from the existing secret classes defined inutil.pyused by GBEK.Secret.from_spec()andSecret.from_option_string().apache_beam.transforms.utilfor backwards compatibility.secret_test.pyand updatedutil_test.py.