Skip to content
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

Merged

Conversation

dinesh-crest
Copy link
Contributor

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

@dinesh-crest dinesh-crest requested review from a team as code owners April 6, 2023 06:11
@snippet-bot
Copy link

snippet-bot bot commented Apr 6, 2023

Here is the summary of changes.

You are about to add 1 region tag.

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

@product-auto-label product-auto-label bot added api: dlp Issues related to the Sensitive Data Protection API. samples Issues that are directly related to samples. labels Apr 6, 2023
// 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

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: required
  • minLikelihood: 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.

Copy link
Contributor Author

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?

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 Show resolved Hide resolved
dlp/inspectWithCustomRegex.js Show resolved Hide resolved
// [END dlp_inspect_custom_regex]
}

main(...process.argv.slice(2));

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.

Copy link
Contributor Author

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.

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).

dlp/inspectWithCustomRegex.js Outdated Show resolved Hide resolved
dlp/inspectWithCustomRegex.js Outdated Show resolved Hide resolved
dlp/system-test/inspect.test.js Outdated Show resolved Hide resolved
@kweinmeister kweinmeister added kokoro:force-run Add this label to force Kokoro to re-run the tests. actions:force-run labels Apr 14, 2023
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 14, 2023
@kweinmeister kweinmeister added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 14, 2023
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 14, 2023
@kweinmeister kweinmeister merged commit 183a16b into GoogleCloudPlatform:main Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: dlp Issues related to the Sensitive Data Protection API. samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants