-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
DLP: Added sample for inspect string with custom regex #3107
DLP: Added sample for inspect string with custom regex #3107
Conversation
Added unit test cases for the same
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
dlp/inspectWithCustomRegex.js
Outdated
// sample-metadata: | ||
// title: Inspects strings | ||
// description: Inspects a string using custom regex pattern. | ||
// usage: node inspectWithCustomRegex.js my-project string minLikelihood maxFindings infoTypes customInfoTypes includeQuote |
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.
Can we trim this down?
string
: requiredminLikelihood
: we should be able to remove this because the likelihood for custom infotype (which is how regex is used) is controlled by the request anyway.maxFindings
: does not make sense in the context of this sample. We're inspecting a small-ish string, not megabytes of file content, so there will only be so many findings. If you want to limit it anyway, just hardcode a limit of ~1000 in the sample. Users can change it if they need to.infoTypes
: Can omit, since we're demonstrating regex matching only. I guess it's possible we may want to show side-by-side detection of custom and built-in infotypes, but if that's the case move this to the end and make it optional. (Also if that is the case, lets make the example string actually demonstrate that)customInfoTypes
: Since the whole point of this sample is to demonstrate regex, we should ask for regex directly and construct the custom infotype in code.includeQuote
: as with maxFindings, lets just set this to true for demo purposes. If users want to change it they can edit the code.
At a high level we should make the sample as easy as possible to run. Adding a lot of parameters and using obscure syntax (such as the ',' and regex/dict hybrid for customInfoTypes) will lead to confusion and frustration.
As a user of this sample, I should be able to say node inspectWithCustomRegex.js 'this is my serial number aab-bcdd-eef' '[a-f\-]10'
and see results.
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.
@soumya92 I had the same thought when I started the implementation but I noticed a particular structure is being followed for all inspect samples. Couldn't figure out the exact reason but mostly it was to keep the sample code consistent. Anyway, I feel your findings look reasonable and so I have updated this sample. Also, will it be okay if I make these same changes in my other PRs?
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 go for it! The easier we make our samples to use, the better
dlp/inspectWithCustomRegex.js
Outdated
// [END dlp_inspect_custom_regex] | ||
} | ||
|
||
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.
This should be after process.on to avoid missing synchronous promise rejections.
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.
Is there any use case where this can happen? I have updated the code as you mentioned but during testing, I found the same results.
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 don't think it happens right now, but I could imagine in the future there might be synchronous validation of requests (e.g. bad enum values might throw before even making a network request).
Added sample for inspect string with custom regex
Added unit test cases for the same
Reference: https://cloud.google.com/dlp/docs/samples/dlp-inspect-custom-regex