add Deid samples and resource #1400

averikitsch · 2018-03-13T23:38:24Z

No description provided.

googlebot · 2018-03-13T23:38:27Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

andrewsg · 2018-03-13T23:43:03Z

dlp/deid.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Sample app that uses the Data Loss Prevention API for deidentifying


Can you think of a wording that shortens this to one line?

andrewsg · 2018-03-13T23:43:30Z

dlp/deid.py

+from __future__ import print_function
+
+import argparse
+


We can remove this blank line, I think.

andrewsg · 2018-03-13T23:45:16Z

dlp/deid.py

+        masking_character: The character to mask matching sensitive data with.
+        number_to_mask: The maximum number of sensitive characters to mask in
+            a match. If omitted the request or set to 0, the API will mask any
+            mathcing characters.


"If omitted or set to zero, the API will default to no maximum."

andrewsg · 2018-03-13T23:45:48Z

dlp/deid.py

+    import google.cloud.dlp
+
+    # Instantiate a client
+    google.cloud.dlp.DlpServiceClient.SERVICE_ADDRESS = 'autopush-dlp.sandbox.googleapis.com' # DO NOT SUBMIT


Go ahead and remove this for the commit. (Actually, I think we don't even need it anymore due to changes on the API side since we started).

Also apply this to the other functions, please.

andrewsg · 2018-03-13T23:47:23Z

dlp/deid.py

+    google.cloud.dlp.DlpServiceClient.SERVICE_ADDRESS = 'autopush-dlp.sandbox.googleapis.com' # DO NOT SUBMIT
+    dlp = google.cloud.dlp.DlpServiceClient()
+
+    # Add parent


For consistency with the inspect_content.py samples, use this comment:

# Convert the project id into a full resource id.

Also apply this to the other functions, please.

andrewsg · 2018-03-14T00:13:39Z

dlp/deid_test.py

+import deid
+
+harmful_string = 'My SSN is 372819127'
+parent = os.environ['GCLOUD_PROJECT']


Call this 'project' instead of 'parent' to avoid confusing it with the fully qualified resource name.

andrewsg · 2018-03-14T00:14:13Z

dlp/deid_test.py

+key_name = os.environ['DLP_DEID_KEY_NAME']
+surrogate_type = 'SSN_TOKEN'
+csv_file = 'resources/dates.csv'
+output_csv_file = 'resources/temp.results.csv'


Check out the inspect_config_test.py file to see how to create a temporary directory, instead of hardcoding a temp file here.

andrewsg · 2018-03-14T00:15:01Z

dlp/deid_test.py

+    assert 'My SSN is *******27' in out
+
+
+def test_deidentify_with_mask_handles_masking_number_error(capsys):


We can go ahead and remove this test; it's not a problem for the sample if the API rejects this.

andrewsg · 2018-03-14T00:15:44Z

dlp/deid_test.py

+    out, _ = capsys.readouterr()
+
+    assert 'Successful' in out
+    # read in csv??


No need to read in the CSV at this time, but go ahead and remove this comment.

andrewsg · 2018-03-14T00:16:10Z

dlp/deid_test.py

+                                        date_fields=date_fields,
+                                        context_field_id=csv_context_field,
+                                        key_name=key_name)
+# [END Deidentify with date shift]


Remove all the START and END tags in the test file; they're only needed in the real sample file. They're used to mark regions that are auto-included in our documentation.

andrewsg · 2018-03-14T00:20:41Z

Great work on this!

theacodes · 2018-03-14T15:54:16Z

dlp/deid.py

+    wrapped_key = base64.b64decode(wrapped_key)
+
+    # Construct Deidentify Config
+    reidentify_config = {


this is a bit wild. @andrewsg wdyt about using the real proto types here?

Yeah, they're all really heavily nested. What's the advantage of using the real proto types vs. just breaking up the dictionaries into separate steps of construction and adding them together iteratively?

It's less about breaking it up into steps and more about knowing the actual structure of the item in question. When using the message types directly you get a better sense for how things are constructed and where you can look to find out additional parameters. With just a big o' nested dictionary it's sort of a wild west - the user is left asking "well, how did they know how to construct this? How do I know how to construct this?".

I agree with this diagnosis of the problem for sure, but I'm not sure about the solution. If we use the real protos here people will just run into the proto help() crash bug. Node doesn't use proto objects; does it have a better user experience for discovering this stuff?

Node is also a vastly different community with different expectations. Remember, Python is strongly moving towards strongly typed. Seeing giant dictionary with no details on the structure or type constraints isn't wonderful.

Proto help() crash bug has been fixed and should hopefully be released soon.

Hmm, okay. I'll buy that. I think that's a bigger change to our focus than we can reasonably do in this PR, though; can we accept this part as-is and do another pass this or next week to see what it looks like with protos instead?

theacodes · 2018-03-14T15:54:54Z

dlp/deid.py

+    item = {'value': string}
+
+    # Call the API
+    response = dlp.reidentify_content(parent,


please avoid hanging indents throughout, prefer to line break at the opening ( and then indent the continuation by one level.

theacodes · 2018-03-14T15:55:40Z

dlp/deid.py

+    # Read and parse the CSV file
+    import csv
+    from datetime import datetime
+    file = []


don't use file, it's a built-in.

theacodes · 2018-03-14T15:56:48Z

dlp/deid.py

+        '-m', '--masking_character',
+        help='The character to mask matching sensitive data with.')
+    mask_parser.add_argument(
+        '-p', '--project', default=os.environ['GCLOUD_PROJECT'])


Please just don't do project ID detection at all. Have them pass it in.

averikitsch · 2018-03-14T16:19:57Z

I signed it!

googlebot · 2018-03-14T16:20:00Z

CLAs look good, thanks!

theacodes · 2018-03-14T16:55:30Z

Sure.

…

On Wed, Mar 14, 2018 at 9:50 AM Andrew Gorcester ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In dlp/deid.py <#1400 (comment)> : > + # Import the client library + import google.cloud.dlp + + # Instantiate a client + google.cloud.dlp.DlpServiceClient.SERVICE_ADDRESS = 'autopush-dlp.sandbox.googleapis.com' + dlp = google.cloud.dlp.DlpServiceClient() + + # Add parent + parent = dlp.project_path(parent) + + # Wrapped key can not be base64 encoded + import base64 + wrapped_key = base64.b64decode(wrapped_key) + + # Construct Deidentify Config + reidentify_config = { Hmm, okay. I'll buy that. I think that's a bigger change to our focus than we can reasonably do in this PR, though; can we accept this part as-is and do another pass this or next week to see what it looks like with protos instead? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1400 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPUc7GVrk5D6Yrn-IKE9Ez7QFFWSbZgks5teUpggaJpZM4Spnkx> .

lesv

LGTM - No changes since this was merged (and Jon and Andrew have reviewed). (And it really looks good)

Your comments improve things in almost all cases, the one place i'd quibble is "Print out the results" which appears to be redundant to the code. (ie if the line below and the comment are the same, then don't bother)

In general, unless the language style for comments differ, it's good to prefer terseness w/o sacrificing clarity. If you haven't already read the Google style guide comments it's worth looking at. (Note - it looks like you have)

I was going to suggest some additional vertical white space, but that appears to be counter the community's view. The one place that appears to be recommended is before the "Returns:"

averikitsch added 2 commits March 13, 2018 16:35

deid samples

fe2581e

added csv file

28c51fc

averikitsch assigned andrewsg Mar 13, 2018

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Mar 13, 2018

andrewsg requested changes Mar 14, 2018

View reviewed changes

theacodes reviewed Mar 14, 2018

View reviewed changes

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Mar 14, 2018

averikitsch added 3 commits March 14, 2018 12:21

pull request comment changes

1af703f

Updated project id as first positional argument

3ed824b

added project to argument list

ebcb63e

averikitsch assigned lesv Mar 15, 2018

andrewsg approved these changes Mar 15, 2018

View reviewed changes

averikitsch merged commit 1decee1 into GoogleCloudPlatform:dlp Mar 15, 2018

lesv reviewed Mar 15, 2018

View reviewed changes

		assert 'My SSN is *******27' in out


		def test_deidentify_with_mask_handles_masking_number_error(capsys):

add Deid samples and resource #1400

add Deid samples and resource #1400

Conversation

averikitsch commented Mar 13, 2018

googlebot commented Mar 13, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsg commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

averikitsch commented Mar 14, 2018

googlebot commented Mar 14, 2018

theacodes commented Mar 14, 2018 via email

lesv left a comment

Choose a reason for hiding this comment