-
Notifications
You must be signed in to change notification settings - Fork 54
[DO NOT MERGE] Propose refined sample format #154
Conversation
efe47c5
to
787b237
Compare
samples/system-test/inspect.test.js
Outdated
const output = await tools.runAsync( | ||
`${cmd} string "I'm Gary and my email is gary@example.com"`, | ||
`node dlp_inspect_string.js ${PROJECT_ID} "I'm Gary and my email is gary@example.com"`, |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
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.
All the sample tests in this repo have that problem.
If we use this for how Node samples should look like, we need to replace |
@jmdobry, what's the status of this PR? |
We'll be finalizing it end of February at a team summit. |
Codecov Report
@@ Coverage Diff @@
## master #154 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 1 1
Lines 4 4
=====================================
Hits 4 4 Continue to review full report at Codecov.
|
samples/dlp_inspect_image_file.js
Outdated
const fs = require('fs'); | ||
|
||
export async function inspectImageFile( | ||
projectId = 'YOUR_PROJECT_ID', |
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.
We should never require the projectId
. Best practice is to leverage the metadata server present on all GCP runtimes.
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.
DLP is somewhat special as the request needs the project ID, users should not pass it in though but use dlp.getProjectId()
, https://github.com/googleapis/nodejs-dlp/blob/master/src/v2/dlp_service_client.js#L268.
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's not actually that special, quite a few APIs require project ID in path parameters to specify the resource owner for billing (different from the inferred project ID used for authentication). Current guidance from the sample working group is to make project ID explicit in the case where it's a required value in the request body. For almost all samples however, the project ID used for authentication should be inferred from the environment (hence the use of const dlp = new DLP.DlpServiceClient();
in the sample)
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.
OK
Note: The failing test is not related to this PR. |
6144768
to
cabcf55
Compare
// [END dlp_inspect_image_file] | ||
} | ||
|
||
main(...process.argv.slice(2)); |
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.
there's been discussion in the Node.js tooling working group about providing a slightly higher level abstraction on process.argv; potentially Node.js would provide an object.
I agree with the decision to drop yargs so that folks don't have a dependency, but I've definitely found it can be a bit confusing for folks as to why they need to perform a slice operation.
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.
Yeah, I made this as simple as possible here while still allowing the sample to be runnable from the command-line. Users on cloud.google.com won't see the slice, they'll just be copying the code between the START and END blocks. I imagine @beccasaurus and co. might auto-generate a more interesting/helpful CLI wrapper.
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.
Yep, we use yargs with --input_param="value"
flags for all sample input parameters
cabcf55
to
5322d38
Compare
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 really like this work to modernize code samples, especially the fact that you've broken everything out into its own files -- as you say this will be great for auto generation.
bac400f
to
cf6666f
Compare
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.
👍
@@ -0,0 +1,47 @@ | |||
/** |
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.
This should be samples/dlp-delete-job.js
per recent @JustinBeckwith PR changes request
All files _
--> -
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.
what's the reason for that? seems it would be easier for our tooling to have the filename be the exact region tag string
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.
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.
A few things!
- JavaScript developers tend to prefer dashes to underscores
- The previous file names were really long and had duplicate data (like the project name...) in them
- I am generally nervous about encoding metadata inside of file names
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.
- So that's why lodash is more popular than underscore.js 😛 I feel this could be answered with BigQuery
- What previous file names?
- The cloud.google.com use case for our samples (where filename doesn't matter) far outweighs the use case where the filename even matters, so we're making things harder for our tooling without much benefit. But if e.g. for Java we already have to have logic to translate region tags to pascal-cased file names, we might as well have more logic to translate region tags to kebab-cased file names for Node.js.
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.
Yeah to be clear - this ain't a hill I want to die on. With no other constraints, I would prefer dashes and not encoding metadata in the file name. If that's a ton of extra work - then I'm not gonna raise a fuss. I trust y'all :)
cf6666f
to
c5e165e
Compare
'use strict'; | ||
|
||
const {assert} = require('chai'); | ||
const dlp = require('@google-cloud/dlp'); |
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.
Should the test use this style...
const {DlpServiceClient} = require('@google-cloud/dlp');
@@ -0,0 +1,59 @@ | |||
/** |
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.
s/system-test/test/g
We should now have tests in ./samples/test
and deprecate the phrase system-test
for our sample tests.
( I think? ) => this is a refactor that @JustinBeckwith did today
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.
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.
( fwiw – I think ./samples/test
is clean. and people understand it. it's idiomatic. )
•
( when I read "system-test" my brain takes a moment to wonder "what types of tests are these?" )
•
( i would support this 🙈 )
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.
Yeah, I tend to just switch these to test
as I go.
Canonical samples have been documented elsewhere. Thanks for review! |
New format based on recent discussions on sample best practices.
Format examples:
Notable changes
...process.argv.slice(2)
which gets passed to the main so the sample can still be invoked from the command-line.Structure of samples
'use strict'
;filepath
, read in the file's bytes into a variable to be added to the request object.async function usefulFunctionNameHere()
:1. Includes inline comments about what the options are for
usefulFunctionNameHere
function, passing in the variables in 2i.main
function, passing in...process.argv.slice(2)
Notes
projectId
can't be hard-coded. The second is a more complicated sample that requires afilepath
(which subsequently requires the file's bytes be read into memory)