Add regional endpoints code samples #12097

jakubrauch · 2024-08-01T15:12:01Z

Description

Fixes: N/A

Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.

Checklist

I have followed Sample Guidelines from AUTHORING_GUIDE.MD
README is updated to include all relevant information
Tests pass: ~~nox -s py-3.9~~ pytest (see Test Environment Setup)
- This class is mostly a copypaste of inspect_string.py, tested using python available on the host
Lint pass: nox -s lint (see Test Environment Setup)
These samples need a new API enabled in testing projects to pass (let us know which ones)
These samples need a new/updated env vars in testing projects set to pass (let us know which ones)
This sample adds a new sample directory, and I updated the CODEOWNERS file with the codeowners for this sample
This sample adds a new Product API, and I updated the Blunderbuss issue/PR auto-assigner with the codeowners for this sample
Please merge this PR for me once it is approved

snippet-bot · 2024-08-01T15:12:11Z

Here is the summary of changes.

You are about to add 1 region tag.

dlp/snippets/Inspect/inspect_string_rep.py:21, tag dlp_inspect_string_rep

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

lxhfirenking · 2024-08-09T20:50:54Z

dlp/snippets/Inspect/inspect_string_rep_test.py

+        GCLOUD_PROJECT,
+        rep_location,
+        test_string,
+        ["FIRST_NAME", "EMAIL_ADDRESS"],


use PERSON_NAME instead.

lxhfirenking · 2024-08-09T20:51:49Z

dlp/snippets/Inspect/inspect_string_rep.py

+        nargs="+",
+        help="Strings representing info types to look for. A full list of "
+        "info categories and types is available from the API. Examples "
+        'include "FIRST_NAME", "LAST_NAME", "EMAIL_ADDRESS". '


Can you change FIRST_NAME LAST_NAME into some other infotypes. They are not recommended in https://cloud.google.com/sensitive-data-protection/docs/infotypes-reference , so we should avoid them in code samples as well.

The goal of a code sample is not to make a fully-runnable wrapper tool around the API, but to provide an example of a specific case. The style guide mentions that we shouldn't be creating CLIs around code samples to reduce maintenance burden.

Instead, the code sample should have a hard-coded request, that people can copy-paste and run directly.

lxhfirenking · 2024-08-09T20:52:08Z

dlp/snippets/Inspect/inspect_string_rep.py

+        help="The Google Cloud project id to use as a parent resource.",
+    )
+    parser.add_argument(
+        "--rep_location",


Add a few examples of what values are valid in the help message.

Also add a default value, since all flags here are optional, but leaving this empty will make the snippet fails to run.

davidcavazos · 2024-08-19T18:59:02Z

dlp/snippets/Inspect/inspect_string_rep.py

+import argparse
+
+# [START dlp_inspect_string_rep]
+from typing import List


As per PEP-585, we can now type hint with list directly (for example list[str]), so no need to import this.

davidcavazos · 2024-08-19T19:06:52Z

dlp/snippets/Inspect/inspect_string_rep.py

+
+def inspect_string(
+    project: str,
+    rep_location: str,


Nit: For naming consistency with other samples, can we keep this as just location?

davidcavazos · 2024-08-19T19:21:32Z

dlp/snippets/Inspect/inspect_string_rep.py

+
+    # Assemble the regional endpoint url using provided rep location
+    rep_endpoint = f"dlp.{rep_location}.rep.googleapis.com"
+    client_options = {"api_endpoint": rep_endpoint}


Nit: It might be a good idea to match the variable name with the option entry? Like {"api_endpoint": api_endpoint}

davidcavazos · 2024-08-19T19:23:17Z

dlp/snippets/Inspect/inspect_string_rep.py

+
+    # Prepare info_types by converting the list of strings into a list of
+    # dictionaries (protos are also accepted).
+    info_types = [{"name": info_type} for info_type in info_types]


From this it's not clear to me what the argument info_types is supposed to look like. For a code sample, it's best if the request is completely hard-coded to something we know works and can be copy-pasted to run.

davidcavazos · 2024-08-19T19:24:00Z

dlp/snippets/Inspect/inspect_string_rep.py

+    # optionally be omitted entirely.
+    inspect_config = {
+        "info_types": info_types,
+        "include_quote": include_quote,


Again, I would hard-code a value for include_quote here. It makes the sample easier to read and there are less decisions to be taken for users.

davidcavazos · 2024-08-19T19:26:13Z

dlp/snippets/Inspect/inspect_string_rep.py

+
+    # Call the API.
+    response = dlp.inspect_content(
+        request={"parent": parent, "inspect_config": inspect_config, "item": item}


This request is a little hard to reason about since all the variables were computed before. It might be a good idea to inline the values directly here instead of having them as separate variables.

davidcavazos · 2024-08-19T19:27:59Z

dlp/snippets/Inspect/inspect_string_rep.py

+    # Print out the results.
+    if response.result.findings:
+        for finding in response.result.findings:
+            try:


Instead of try-catch, we could use hasattr to check that.

if hasattr(finding, 'quote'): print(f"Quote: {finding.quote}")

davidcavazos · 2024-08-19T19:28:41Z

dlp/snippets/Inspect/inspect_string_rep.py

+
+    parser.add_argument("item", help="The string to inspect.")
+    parser.add_argument(
+        "--project",


Can we mark this as required=True?

davidcavazos · 2024-08-19T19:32:13Z

dlp/snippets/Inspect/inspect_string_rep.py

+        nargs="+",
+        help="Strings representing info types to look for. A full list of "
+        "info categories and types is available from the API. Examples "
+        'include "FIRST_NAME", "LAST_NAME", "EMAIL_ADDRESS". '


The goal of a code sample is not to make a fully-runnable wrapper tool around the API, but to provide an example of a specific case. The style guide mentions that we shouldn't be creating CLIs around code samples to reduce maintenance burden.

Instead, the code sample should have a hard-coded request, that people can copy-paste and run directly.

davidcavazos · 2024-08-19T19:33:58Z

dlp/snippets/Inspect/inspect_string_rep_test.py

+        include_quote=True,
+    )
+
+    out, _ = capsys.readouterr()


Testing on stdout is brittle and can cause tests to fail if the underlying implementation changes. Instead, you should return the response from the code snippet and test it directly here.

davidcavazos · 2024-08-22T23:40:29Z

dlp/snippets/Inspect/inspect_string_rep.py

+
+# [END dlp_inspect_string_rep]
+
+if __name__ == "__main__":


Please consider removing the entire CLI interface. We shouldn't include CLI interfaces unless they're strictly necessary for the code inside the region tags to work.

glasnt · 2025-02-06T15:46:54Z

Hi @jakubrauch, friendly ping that we're waiting on you to address the code review comments for this PR.

Since this PR is a bit older, we've recently introduced Python 3.13 testing; please rebase your PR to main to ensure your code will be tested against this new version.

Add regional endpoints code samples

d282439

jakubrauch requested review from a team as code owners August 1, 2024 15:12

product-auto-label bot added samples Issues that are directly related to samples. api: dlp Issues related to the Sensitive Data Protection API. labels Aug 1, 2024

blunderbuss-gcf bot assigned ivanmed Aug 1, 2024

lxhfirenking requested changes Aug 9, 2024

View reviewed changes

davidcavazos reviewed Aug 19, 2024

View reviewed changes

davidcavazos reviewed Sep 6, 2024

View reviewed changes

jakubrauch assigned jakubrauch and unassigned ivanmed Dec 9, 2024

glasnt added the waiting-response Waiting for the author's response. label Mar 5, 2025

Add regional endpoints code samples #12097

Are you sure you want to change the base?

Add regional endpoints code samples #12097

Uh oh!

Conversation

jakubrauch commented Aug 1, 2024

Description

Checklist

Uh oh!

snippet-bot bot commented Aug 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glasnt commented Feb 6, 2025

Uh oh!

Uh oh!