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.

Sign up for GitHub

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

Jump to bottom

Add new DLP samples (Bigquery, DeID, Risk Analysis) #841

Merged

jabubake merged 13 commits into master from dlp-bq

Oct 19, 2017

Contributor

ace-n commented Sep 6, 2017 •

edited

Loading

Do not merge until this code has been:
~~a) tested~~
~~b) updated to use new client libraries (as the current ones don't support scanning BigQuery)~~

Reminder: this should be reviewed on a best-effort basis.


          UNTESTED: Add DLP BigQuery sample

7f531ae

ace-n added the do not merge label

ace-n requested a review from jabubake

September 6, 2017 22:52

googlebot added the cla: yes label

Ace Nassri added 3 commits

October 17, 2017 23:18


          Update DLP dependency

eca2c8b


          Fix inspectBQ tests

3c8a224


          Add DeID + RiskAnalysis samples

2a6148b

ace-n changed the title ~~UNTESTED/DO NOT MERGE: Add DLP BigQuery sample~~ Add new DLP samples (Bigquery, DeID, Risk Analysis)


          Merge branch 'master' into dlp-bq

28ae829

Contributor Author

ace-n commented Oct 18, 2017

This can be merged now:
a) the new Maven libraries support the needed methods
b) tests have been added, and they pass

Note: I did not review the API/these samples for Java-specific developer experience concerns (e.g. idiomaticity). I simply wrote them so that the tests passed. (@jabubake let me know if you'd like me to make any changes.)

ace-n removed the do not merge label

jabubake suggested changes

View reviewed changes

Contributor

jabubake left a comment

Please run the google-java-formatter(https://github.com/google/google-java-format) on these files.
Also please run mvn clean verify locally ensuring that the tests pass for java-docs-samples-testing project.

dlp/src/main/java/com/example/dlp/DeId.java Outdated

+              import com.google.cloud.dlp.v2beta1.DlpServiceClient;
+              import com.google.common.io.BaseEncoding;
+              import com.google.privacy.dlp.v2beta1.*;

Contributor

jabubake Oct 18, 2017

Please avoid .* imports

Contributor Author

ace-n Oct 18, 2017

Done.

dlp/src/main/java/com/example/dlp/DeId.java Outdated

+              import com.google.privacy.dlp.v2beta1.InfoTypeTransformations.InfoTypeTransformation;
+              import com.google.privacy.dlp.v2beta1.CryptoReplaceFfxFpeConfig.FfxCommonNativeAlphabet;
+              import com.google.protobuf.ByteString;
+              import org.apache.commons.cli.*;

Contributor

jabubake Oct 18, 2017

Please avoid .* imports

Contributor Author

ace-n Oct 18, 2017

Done.

dlp/src/main/java/com/example/dlp/DeId.java Outdated

+              import com.google.protobuf.ByteString;
+              import org.apache.commons.cli.*;
+              public class DeId {

Contributor

jabubake Oct 18, 2017

Given this short name, I recommend renaming to DeIdentification

Contributor Author

ace-n Oct 18, 2017

Done.

dlp/src/main/java/com/example/dlp/DeId.java Outdated


		public class DeId {

		private static void deidentifyWithMask(String string, Character maskingCharacter, int numberToMask) {

Contributor

jabubake Oct 18, 2017

deidentify => deIdentify

Contributor Author

ace-n Oct 18, 2017

Done.

dlp/src/main/java/com/example/dlp/DeId.java Outdated

+                            .setPrimitiveTransformation(primitiveTransformation)
+                            .build();
+                    InfoTypeTransformations infoTypeTransformationArray =

Contributor

jabubake Oct 18, 2017

Given all these objects that get created just to create a request, it would be to provide a comment above each to state what it is. It is unfortunate that InfoTypeTransformations object exists to just provide an InfoTypeTransformation[] surface. PrimitiveTransformation, InfoTypeTransformation also seem pretty confusing. In an ideal world, PrimitveTransformation, InfoTypeTransformation, InfoTypeTransformations are un-necessary objects.

Contributor Author

ace-n Oct 18, 2017

I had similar comments with the Node client libraries, to be honest. I'll follow up with the API/product team(s) about this.

(TODO, so I can CTRL-F for this comment.)

dlp/src/main/java/com/example/dlp/DeId.java Outdated

+                            .setValue(string)
+                            .build();
+                    KmsWrappedCryptoKey kmsWrappedCryptoKey =

Contributor

jabubake Oct 18, 2017

Same comment as earlier, better comments around these objects and ideally some of these can get cleaned up on the surface. This looks very cumbersome to the user.

dlp/src/main/java/com/example/dlp/Inspect.java Outdated

@@ @@ -20,24 +20,8 @@ @@
               import com.google.cloud.ServiceOptions;
               import com.google.cloud.dlp.v2beta1.DlpServiceClient;
               import com.google.longrunning.Operation;
-              import com.google.privacy.dlp.v2beta1.CloudStorageOptions;
+              import com.google.privacy.dlp.v2beta1.*;

Contributor

jabubake Oct 18, 2017

Please remove .* import.

Contributor Author

ace-n Oct 18, 2017

Done.

dlp/src/test/java/com/example/dlp/DeIdIT.java Outdated

+              import static org.junit.Assert.*;
+              @RunWith(JUnit4.class)
+              public class DeIdIT {

Contributor

jabubake Oct 18, 2017

DeIdentificationIT

Contributor Author

ace-n Oct 18, 2017

Done.

dlp/src/test/java/com/example/dlp/DeIdIT.java Outdated

+              import java.util.regex.Matcher;
+              import java.util.regex.Pattern;
+              import static org.junit.Assert.*;

Contributor

jabubake Oct 18, 2017

Please remove .* import

Contributor Author

ace-n Oct 18, 2017

Done.

dlp/src/test/java/com/example/dlp/RiskAnalysisIT.java

+                  out = new PrintStream(bout);
+                  System.setOut(out); // TODO(b/64541432) DLP currently doesn't support GOOGLE DEFAULT AUTH
+                  assertNotNull(System.getenv("GOOGLE_APPLICATION_CREDENTIALS"));
+                  assertNotNull(System.getenv("DLP_DEID_WRAPPED_KEY"));

Contributor

jabubake Oct 18, 2017

if you can hard-code these variables, that would be ideal.
Else .travis.yaml will need to be updated so that these tests are successful on java-docs-samples-testing.

Contributor Author

ace-n Oct 18, 2017 •

edited

Loading

I'm assuming you're referring to the DLP_DEID_* variables and not GOOGLE_APPLICATION_CREDENTIALS.

DLP_DEID_WRAPPED_KEY is an encrypted secret - it technically should be safe to hard-code, but I wouldn't call it a 'best practice'. (As far as I'm aware, the best practice for such secrets is to add them to CI as secret environment variables.)

DLP_DEID_KEY_NAME is not terribly secret, but would reveal your project ID and keyring name if hard-coded.

The Node samples usually use environment variables for such things, but I can hard-code these if you want. Alternatively, I'm happy (and would prefer) to add these variables to your Travis builds (through the web UI, since they're supposed to remain secret - and not travis.yaml).

Contributor

jabubake Oct 18, 2017

In that case, agree on not hard-coding. @lesv might be able to recommend the best approach.

Contributor

lesv Oct 18, 2017

Ace - can you just add how to do this to the README. I'm a fan of getting this stuff from Datastore, but in this case I'm going to go w/ Ace's current implementation.

We should also mention it in the root README as well - once we write it.

Contributor Author

ace-n Oct 19, 2017

Done.

Ace Nassri added 2 commits

October 18, 2017 11:39


          Address comments: clean up imports, document DeId requests, other mis…

7e439bc

…c fixes


          Add missing comment

18038cb

lesv reviewed

View reviewed changes

dlp/src/main/java/com/example/dlp/Inspect.java

+                  // Instantiates a client
+                  try (DlpServiceClient dlpServiceClient = DlpServiceClient.create()) {
+                    // (Optional) The project ID to run the API call under

Contributor

lesv Oct 18, 2017

You might wish to mention the API's if you are going to mention this much stuff.

Contributor

lesv Oct 18, 2017

Perhaps in a more JavaDoc way and move the tag earlier.

Contributor Author

ace-n Oct 18, 2017

Done.

lesv reviewed

View reviewed changes

Contributor

lesv left a comment

Be sure to do a "mvn verify" or look at the results from Circle to see how you are doing on the Java Style guide - I see stuff that looks a bit iffy.

lesv reviewed

View reviewed changes

dlp/src/test/java/com/example/dlp/DeIdentificationIT.java

+                private PrintStream out;
+                // Update to wrapped local encryption key
+                private String wrappedKey = System.getenv("DLP_DEID_WRAPPED_KEY");

Contributor

lesv Oct 18, 2017

We'll need to update our testing if this stays.

Contributor Author

ace-n Oct 18, 2017

I've added these environment variables to Travis, but they (and a few BigQuery tables) are based off of the nodejs-docs-samples project - and will need to be moved over at some point.

lesv suggested changes

View reviewed changes

Contributor

lesv left a comment

Please update the README to mention your environment variables for testing.

Ace Nassri added 5 commits

October 18, 2017 14:39


          Add env vars to README.md

f7ba95e


          Switch to Javadoc-style comments

bd6eb95


          (Temporarily) point BQ tests at nodejs-docs-samples GCP project

b5ebdab


          Remove nodejs-docs-samples project ID for BigQuery tests

2c4bc7d


          Address lint comments

f34ddd4

Contributor Author

ace-n commented Oct 19, 2017

mvn verify comments have been fixed (with one exception, see below).

I didn't address the "abbreviation" linter warning, since it was complaining about files that I didn't modify - but let me know if this was in error.

jabubake approved these changes

View reviewed changes


          Merge branch 'master' into dlp-bq

30f0028

jabubake merged commit b14e6c9 into master

jabubake deleted the dlp-bq branch

October 19, 2017 16:51

minherz pushed a commit that referenced this pull request


          chore(main): release 0.122.22 (#841)

73f72e4

🤖 I have created a release *beep* *boop*
---


### [0.122.22](googleapis/java-errorreporting@v0.122.21...v0.122.22) (2022-03-29)


### Dependencies

* update dependency com.google.cloud:google-cloud-core to v2.5.11 ([#839](googleapis/java-errorreporting#839)) ([ecda8aa](googleapis/java-errorreporting@ecda8aa))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v2.9.0 ([#840](googleapis/java-errorreporting#840)) ([7f59117](googleapis/java-errorreporting@7f59117))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

minherz pushed a commit that referenced this pull request


          chore(main): release 0.122.22 (#841)

e007c01

🤖 I have created a release *beep* *boop*
---


### [0.122.22](googleapis/java-errorreporting@v0.122.21...v0.122.22) (2022-03-29)


### Dependencies

* update dependency com.google.cloud:google-cloud-core to v2.5.11 ([#839](googleapis/java-errorreporting#839)) ([ecda8aa](googleapis/java-errorreporting@ecda8aa))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v2.9.0 ([#840](googleapis/java-errorreporting#840)) ([7f59117](googleapis/java-errorreporting@7f59117))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

minherz pushed a commit that referenced this pull request


          chore(main): release 0.122.22 (#841)

1d29955

🤖 I have created a release *beep* *boop*
---


### [0.122.22](googleapis/java-errorreporting@v0.122.21...v0.122.22) (2022-03-29)


### Dependencies

* update dependency com.google.cloud:google-cloud-core to v2.5.11 ([#839](googleapis/java-errorreporting#839)) ([ecda8aa](googleapis/java-errorreporting@ecda8aa))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v2.9.0 ([#840](googleapis/java-errorreporting#840)) ([7f59117](googleapis/java-errorreporting@7f59117))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

Shabirmean pushed a commit that referenced this pull request


          chore(deps): update dependency com.google.cloud:libraries-bom to v25.…

b280825

…2.0 (#841)

[![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [com.google.cloud:libraries-bom](https://cloud.google.com/java/docs/bom) ([source](https://togithub.com/GoogleCloudPlatform/cloud-opensource-java)) | `25.1.0` -> `25.2.0` | [![age](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.2.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.2.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.2.0/compatibility-slim/25.1.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.2.0/confidence-slim/25.1.0)](https://docs.renovatebot.com/merge-confidence/) |

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Renovate will not automatically rebase this PR, because other commits have been found.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-language).

Shabirmean mentioned this pull request

samples: migrate code from googleapis/java-language #7436

Merged

9 tasks

Shabirmean pushed a commit that referenced this pull request


          chore(deps): update dependency com.google.cloud:libraries-bom to v25.…

2f0ec2a

…2.0 (#841)

[![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [com.google.cloud:libraries-bom](https://cloud.google.com/java/docs/bom) ([source](https://togithub.com/GoogleCloudPlatform/cloud-opensource-java)) | `25.1.0` -> `25.2.0` | [![age](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.2.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.2.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.2.0/compatibility-slim/25.1.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.2.0/confidence-slim/25.1.0)](https://docs.renovatebot.com/merge-confidence/) |

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Renovate will not automatically rebase this PR, because other commits have been found.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-language).

Sita04 pushed a commit that referenced this pull request


          samples: Add new DLP samples (Bigquery, DeID, Risk Analysis) (#841)

5c11dea

Sita04 pushed a commit that referenced this pull request


          deps: update dependency com.google.cloud:google-cloud-pubsub to v1.11…

843aefd

…6.0 (#841)

* deps: update dependency com.google.cloud:google-cloud-pubsub to v1.116.0

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

Sita04 pushed a commit that referenced this pull request


          chore(main): release 3.2.0 (#850)

4e96696

🤖 I have created a release *beep* *boop*
---


## [3.2.0](googleapis/java-dlp@v3.1.4...v3.2.0) (2022-03-29)


### Features

* new Bytes and File types: POWERPOINT and EXCEL ([#848](googleapis/java-dlp#848)) ([ffb764d](googleapis/java-dlp@ffb764d))


### Dependencies

* update dependency com.google.cloud:google-cloud-pubsub to v1.116.0 ([#841](googleapis/java-dlp#841)) ([e12a43f](googleapis/java-dlp@e12a43f))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v2.9.0 ([#849](googleapis/java-dlp#849)) ([7106010](googleapis/java-dlp@7106010))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels