-
Notifications
You must be signed in to change notification settings - Fork 58
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
move google-cloud/dlp to peer dependency #25
base: main
Are you sure you want to change the base?
Conversation
4527157
to
fad9d5b
Compare
fad9d5b
to
10f3b1c
Compare
@chaitanya-solvvy @JaredBett Sorry for direct mention, not sure why checks aren't running on this PR to |
I think there's one problem with that change. "use strict";
Object.defineProperty(exports, "__esModule", { value: true });
const SyncCompositeRedactor_1 = require("./SyncCompositeRedactor");
exports.SyncRedactor = SyncCompositeRedactor_1.SyncCompositeRedactor;
const AsyncCompositeRedactor_1 = require("./AsyncCompositeRedactor");
exports.AsyncRedactor = AsyncCompositeRedactor_1.AsyncCompositeRedactor;
const GoogleDLPRedactor_1 = require("./custom/GoogleDLPRedactor");
exports.GoogleDLPRedactor = GoogleDLPRedactor_1.GoogleDLPRedactor;
//# sourceMappingURL=index.js.map As you can see it also load the GoogleDLPRedactor in this entry point and I believe this will fail if google cloud dlp is not installed, even if it's not actively used. |
as for the checks not running: This seems to be an unfortunate limitation of github actions as of right now. Seems like the creator of the PR (@tbjers) has to be also part of the beta in order for the actions to run (see https://github.community/t5/GitHub-API-Development-and/Run-a-GitHub-action-on-pull-request-for-PR-opened-from-a-forked/td-p/15426). |
@geekflyer I think I am part of the beta now. I can see the Actions tab on a few of my repos. |
@geekflyer How do you test this? Is this something we can automate? Or, at least something I can do locally to ensure that my changes are working as intended? |
- @google-cloud/dlp should only be included if necessary
idk. I think that's not that trivial to test automatically (honestly the value isn't probably worth the complexity of that). |
@geekflyer I have pushed updates to the PR. Can you please review? |
Hey, I still need you to address #25 (comment) . |
8c175ff
to
c56a87a
Compare
@geekflyer I have adjusted according to your recommendations. Thanks for being patient with me. |
@JaredBett what do you think of this PR ? I think we should be good to merge this. We probably should bump the minor or possibly even the major, since this technically a breaking change (even for us) and we need to adapt our internal use of redact-pii in the sense that we need to add the DLP library (@google-cloud/dlp) explicitly to our apps then, but overall this seems like a good idea. Also this would allow #29 to be merged without too much trouble. |
6d86a3f
to
351fdf6
Compare
@geekflyer I have fixed the issues previously outlined, and it now passes checks in my fork. |
- upgrade lodash to avoid vulnerabilities - upgrade TypeScript to support newer version of GCDLP - ensure everything still works in Node 8
@geekflyer @JaredBett Any thoughts on this PR please? |
Created a new repository and published a new package till the time this PR is merged. Thanks @tbjers for the PR. |
Can't believe this PR has been sitting here for two years now. Thanks for reviving, @tHBp. |
The Google Cloud DLP package contains a lot of dependencies, as such, if it isn't actively used, it shouldn't be included in the dependencies. Moved to peer dependencies. For instance, when using
redact-pii
in a Lambda function, size increases by at least 10 MB from the Google Cloud DLP package.