-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat(bigquery): expose customer managed encryption key for ML models #9302
feat(bigquery): expose customer managed encryption key for ML models #9302
Conversation
@@ -25,6 25,7 @@ | |||
from google.api_core import datetime_helpers | |||
from google.cloud.bigquery import _helpers | |||
from google.cloud.bigquery_v2 import types | |||
from google.cloud.bigquery.table import EncryptionConfiguration |
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.
It seems wrong for EncryptionConfiguration
to live in the table
module now that it's used for tables and models. Let's create a new encryption_configuration
model for this to live. (Similar to external_config
.)
We can make an alias in the table
module to keep this from being a breaking change.
@@ -21,6 21,8 @@ | |||
import google.cloud._helpers | |||
from google.cloud.bigquery_v2.gapic import enums | |||
|
|||
KMS_KEY_NAME = "projects/1/locations/global/keyRings/1/cryptoKeys/1" |
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.
I know this is a unit test, but we're supposed to discourage use of global
location. Let's change this to us
.
@tswast PTAL. |
…hange location in key
@@ -70,99 70,6 @@ def _verifySchema(self, schema, resource): | |||
self._verify_field(field, r_field) | |||
|
|||
|
|||
class TestEncryptionConfiguration(unittest.TestCase): |
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.
Can we please keep one test to ensure that from google.cloud.bigquery.table import EncryptionConfiguration
continues to work? I don't want to make this feature into breaking change.
bigquery/tests/unit/test_table.py
Outdated
@@ -1118,7 1046,9 @@ def test_clustering_fields_setter_w_none_noop(self): | |||
self.assertFalse("clustering" in table._properties) | |||
|
|||
def test_encryption_configuration_setter(self): | |||
from google.cloud.bigquery.table import EncryptionConfiguration | |||
from google.cloud.bigquery.encryption_configuration import ( |
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.
I meant that we need to test that the old location needs to continue to work.
from google.cloud.bigquery.encryption_configuration import ( | |
# Previously, the EncryptionConfiguration class was in the table module, not the | |
# encryption_configuration module. It was moved to support models encryption. | |
# This test import from the table module to ensure that the previous location | |
# continues to function as an alias. | |
from google.cloud.bigquery.table import ( |
Fixes: #9251