- Notifications
You must be signed in to change notification settings - Fork1.6k
feat(bigquery): expose customer managed encryption key for ML models#9302
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 ourterms of service andprivacy 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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| fromgoogle.api_coreimportdatetime_helpers | ||
| fromgoogle.cloud.bigqueryimport_helpers | ||
| fromgoogle.cloud.bigquery_v2importtypes | ||
| fromgoogle.cloud.bigquery.tableimportEncryptionConfiguration |
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 forEncryptionConfiguration to live in thetable module now that it's used for tables and models. Let's create a newencryption_configuration model for this to live. (Similar toexternal_config.)
We can make an alias in thetable module to keep this from being a breaking change.
| importgoogle.cloud._helpers | ||
| fromgoogle.cloud.bigquery_v2.gapicimportenums | ||
| 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 ofglobal location. Let's change this tous.
HemangChothani commentedOct 11, 2019
@tswast PTAL. |
…hange location in key
| self._verify_field(field,r_field) | ||
| classTestEncryptionConfiguration(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 thatfrom google.cloud.bigquery.table import EncryptionConfiguration continues to work? I don't want to make this feature into breaking change.
Uh oh!
There was an error while loading.Please reload this page.
bigquery/tests/unit/test_table.py Outdated
| deftest_encryption_configuration_setter(self): | ||
| fromgoogle.cloud.bigquery.tableimportEncryptionConfiguration | ||
| fromgoogle.cloud.bigquery.encryption_configurationimport ( |
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.
| fromgoogle.cloud.bigquery.encryption_configurationimport ( | |
| # 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. | |
| fromgoogle.cloud.bigquery.tableimport ( |
Uh oh!
There was an error while loading.Please reload this page.
Fixes:#9251