Move DLP from REST to gRPC + client libraries #413

ace-n · 2017-06-30T19:33:00Z

No description provided.

Change-Id: Iaa03400842667a478e9e29e87ba36fcbda257d60

theacodes · 2017-06-30T19:37:18Z

dlp/inspect.js

-      'Authorization': `Bearer ${authToken}`,
-      'Content-Type': 'application/json'
+  // Create a GCS File inspection job, and resolve it using promises
+  // TODO: is the client library going to handle pagination if no user settings are specified?


Change-Id: I7fdc39e1528dd222c6795c70cce972efc5c60e1c

theacodes · 2017-06-30T19:39:58Z

dlp/inspect.js

-    .then((findingsBody) => {
-      const findings = findingsBody.result.findings;
-      console.log(JSON.stringify(findings, null, 2));
+  // Create a GCS File inspection job, and resolve it using event handlers


The latter part of this comment seems unhelpful. Would it be better to say:

// Create an operation to instance the data in Google Cloud Storage, // wait for it to complete, then list the findings.

theacodes · 2017-06-30T19:40:16Z

dlp/inspect.js

+          // TODO this doesn't produce meaningful results
+        });
+
+        // Handle job errors


This comment is obvious.

theacodes · 2017-06-30T19:40:29Z

dlp/inspect.js

+          return resolve(completeJobResponse);
+        });
+
+        // Handle changes in job metadata (e.g. progress updates)


This comment is also obvious.

theacodes · 2017-06-30T19:40:38Z

dlp/inspect.js

+
+        // Handle changes in job metadata (e.g. progress updates)
+        operation.on('progress', (metadata) => {
+          // TODO this doesn't produce meaningful results


If this does nothing, why not just leave it out?

This provides (and prints out) semi-meaningful results now.

theacodes

This looks mostly fine except for the TODOs and the mostly obvious comments.

Comments should say why you're doing something, not what you're doing.

samtstern · 2017-06-30T19:44:34Z

dlp/inspect.js

    })
    .catch((err) => {
-      console.log('Error in inspectString:', err);
+      console.log('Error in inspectString:', err.message || err);


nit: Why not: Error in inspectString: ${err.message || err} to be consistent with other logging style.

Same comment throughout, since this catch block is copy-pasted

Done - will address.

samtstern · 2017-06-30T19:48:51Z

dlp/inspect.js

+  dlp.createInspectOperation(request)
+    .then((createJobResponse) => {
+      const operation = createJobResponse[0];
+      return new Promise((resolve, reject) => {


What's the difference between this construct and just operation.promise() which you used above?

This uses event handlers - and I wanted to avoid "callback hell".

(If you think the nested callback approach would be better idiomatically vs. mixing promises and event handlers, I'm happy to switch to it.)

samtstern · 2017-06-30T19:51:09Z

dlp/metadata.js

+    category: category,
+    languageCode: languageCode
+  })
+  .then((body) => {


What is the type of body? In other then blocks you use more descriptive names like createJobResponse

Perhaps apiResponse would be a better name here - WDYT?

(Something like listInfoTypesResponse seems a little long-winded.)

Either one SGTM

samtstern · 2017-06-30T19:51:37Z

dlp/metadata.js

+  })
+  .then((body) => {
+    const infoTypes = body[0].infoTypes;
+    console.log(`Info types for category ${category}:`);


Any possibility of infoTypes being empty?

Having a valid category without valid info types is the only way I can see to trigger this. (Invalid categories are detected outside this sample and the resulting errors are handled correctly.)

codecov-io · 2017-06-30T21:51:58Z

Codecov Report

Merging #413 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #413   +/-   ##
=======================================
  Coverage   83.88%   83.88%           
=======================================
  Files           4        4           
  Lines         422      422           
=======================================
  Hits          354      354           
  Misses         68       68

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48733aa...b4d30ae. Read the comment docs.

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: sofisl <55454395+sofisl@users.noreply.github.com>

🤖 I have created a release *beep* *boop* --- ## [4.1.4](https://github.com/googleapis/nodejs-secret-manager/compare/v4.1.3...v4.1.4) (2022-11-11) ### Bug Fixes * **deps:** Use google-gax v3.5.2 ([#413](https://github.com/googleapis/nodejs-secret-manager/issues/413)) ([1652509](https://github.com/googleapis/nodejs-secret-manager/commit/16525099e4a28b466ebfa067a001124d253c8992)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

* Move DLP from REST to gRPC + client libraries * Update package.json * Address comments

Move DLP from REST to gRPC + client libraries

bb6c13e

Change-Id: Iaa03400842667a478e9e29e87ba36fcbda257d60

ace-n requested review from theacodes and jmdobry June 30, 2017 19:33

theacodes reviewed Jun 30, 2017

View reviewed changes

Update package.json

27cb09c

Change-Id: I7fdc39e1528dd222c6795c70cce972efc5c60e1c

theacodes reviewed Jun 30, 2017

View reviewed changes

dlp/inspect.js Outdated

// TODO this doesn't produce meaningful results

});

// Handle job errors

Copy link

theacodes Jun 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is obvious.

theacodes reviewed Jun 30, 2017

View reviewed changes

theacodes approved these changes Jun 30, 2017

View reviewed changes

samtstern reviewed Jun 30, 2017

View reviewed changes

Address comments

b4d30ae

samtstern approved these changes Jun 30, 2017

View reviewed changes

ace-n merged commit bf789f6 into master Jun 30, 2017

ace-n deleted the dlp-vtk branch June 30, 2017 21:57

NimJay pushed a commit that referenced this pull request Nov 11, 2022

chore: release 2.2.0 (#413)

ace21f7

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

NimJay mentioned this pull request Nov 11, 2022

migrate code from googleapis/nodejs-kms #2839

Merged

ace-n pushed a commit that referenced this pull request Nov 11, 2022

chore: release 2.2.0 (#413)

b272847

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

ahrarmonsur pushed a commit that referenced this pull request Nov 16, 2022

chore: release 3.9.0 (#413)

64375fd

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: sofisl <55454395+sofisl@users.noreply.github.com>

ahrarmonsur pushed a commit that referenced this pull request Nov 16, 2022

chore: release 3.9.0 (#413)

ff61522

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: sofisl <55454395+sofisl@users.noreply.github.com>

ahrarmonsur pushed a commit that referenced this pull request Nov 16, 2022

chore: release 3.9.0 (#413)

dcb990c

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: sofisl <55454395+sofisl@users.noreply.github.com>

ahrarmonsur pushed a commit that referenced this pull request Nov 16, 2022

chore: release 3.9.0 (#413)

98640e7

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: sofisl <55454395+sofisl@users.noreply.github.com>

ahrarmonsur pushed a commit that referenced this pull request Nov 16, 2022

chore: release 3.9.0 (#413)

80ec4b5

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: sofisl <55454395+sofisl@users.noreply.github.com>

ahrarmonsur mentioned this pull request Nov 16, 2022

migrate code from googleapis/nodejs-asset #2856

Merged

ace-n pushed a commit that referenced this pull request Nov 17, 2022

chore: release 3.9.0 (#413)

57c3140

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: sofisl <55454395+sofisl@users.noreply.github.com>

Shabirmean pushed a commit that referenced this pull request Feb 16, 2023

Move DLP from REST to gRPC + client libraries (#413)

92b4b4f

* Move DLP from REST to gRPC + client libraries * Update package.json * Address comments

Shabirmean mentioned this pull request Feb 16, 2023

samples: migrate code from googleapis/nodejs-dlp #3035

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move DLP from REST to gRPC + client libraries #413

Move DLP from REST to gRPC + client libraries #413

ace-n commented Jun 30, 2017

theacodes Jun 30, 2017

theacodes Jun 30, 2017

theacodes Jun 30, 2017

theacodes Jun 30, 2017

theacodes Jun 30, 2017

ace-n Jun 30, 2017

theacodes left a comment

samtstern Jun 30, 2017

samtstern Jun 30, 2017

ace-n Jun 30, 2017

samtstern Jun 30, 2017

ace-n Jun 30, 2017

samtstern Jun 30, 2017

ace-n Jun 30, 2017

samtstern Jun 30, 2017

samtstern Jun 30, 2017

ace-n Jun 30, 2017

codecov-io commented Jun 30, 2017

Move DLP from REST to gRPC + client libraries #413

Move DLP from REST to gRPC + client libraries #413

Conversation

ace-n commented Jun 30, 2017

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

theacodes left a 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

codecov-io commented Jun 30, 2017

Codecov Report