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

move google-cloud/dlp to peer dependency #25

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

tbjers
Copy link

@tbjers tbjers commented May 9, 2019

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.

@tbjers tbjers force-pushed the google-cloud-dlp-peer-dependency branch from 4527157 to fad9d5b Compare May 15, 2019 15:39
@tbjers tbjers force-pushed the google-cloud-dlp-peer-dependency branch from fad9d5b to 10f3b1c Compare May 15, 2019 15:41
@tbjers
Copy link
Author

tbjers commented May 15, 2019

@chaitanya-solvvy @JaredBett Sorry for direct mention, not sure why checks aren't running on this PR to redact-pii.

.github/main.workflow Outdated Show resolved Hide resolved
.github/main.workflow Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@geekflyer
Copy link
Contributor

geekflyer commented May 15, 2019

I think there's one problem with that change.
The compiled lib/index.js which is being loaded when you require('redact-pii') contains:

"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.
So in order to really make the google cloud dlp stuff optional we have to also remove it from the module entrypoint (src/index.ts) and update the docs accordingly

@geekflyer
Copy link
Contributor

geekflyer commented May 15, 2019

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).
I'll find a way to run them myself or maybe manually once the MR is in good shape if you can't get into the actions beta until then.

@tbjers
Copy link
Author

tbjers commented May 15, 2019

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).
I'll find a way to run them myself or maybe manually once the MR is in good shape if you can't get into the actions beta until then.

@geekflyer I think I am part of the beta now. I can see the Actions tab on a few of my repos.

@tbjers
Copy link
Author

tbjers commented May 15, 2019

I think there's one problem with that change.
The compiled lib/index.js which is being loaded when you require('redact-pii') contains:

@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
@geekflyer
Copy link
Contributor

geekflyer commented May 15, 2019

I think there's one problem with that change.
The compiled lib/index.js which is being loaded when you require('redact-pii') contains:

@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?

idk. I think that's not that trivial to test automatically (honestly the value isn't probably worth the complexity of that).
You should be able to test it manually though by: building the package locally via npm run build, uninstall google cloud dlp via npm uninstall and then write some small dummy script in which you directly import the compiled the index.js via require('lib/index') then run that dummy script via node dummy.js . I bet this fails currently.

@tbjers
Copy link
Author

tbjers commented May 17, 2019

@geekflyer I have pushed updates to the PR. Can you please review?

@geekflyer
Copy link
Contributor

geekflyer commented May 18, 2019

Hey, I still need you to address #25 (comment) .
Please also undo the changes in GoogleDLPRedactor since they're not necessary if you remove GoogleDLPRedactor from the module entry point.
Just to avoid confusion what I mean: module entry point === src/index.ts.

@tbjers tbjers force-pushed the google-cloud-dlp-peer-dependency branch from 8c175ff to c56a87a Compare May 20, 2019 21:08
@tbjers
Copy link
Author

tbjers commented May 20, 2019

Hey, I still need you to address #25 (comment) .
Please also undo the changes in GoogleDLPRedactor since they're not necessary if you remove GoogleDLPRedactor from the module entry point.

@geekflyer I have adjusted according to your recommendations. Thanks for being patient with me.

@tbjers tbjers closed this Jun 29, 2019
@geekflyer geekflyer reopened this Jul 21, 2019
@geekflyer
Copy link
Contributor

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

@tbjers tbjers force-pushed the google-cloud-dlp-peer-dependency branch from 6d86a3f to 351fdf6 Compare December 17, 2019 17:49
@tbjers
Copy link
Author

tbjers commented Dec 17, 2019

@geekflyer I have fixed the issues previously outlined, and it now passes checks in my fork.

See here: https://github.com/tbjers/redact-pii/commit/351fdf66c1a7e6916fcc832cb6ef9cb55918d7e5/checks?check_suite_id=363104856

Torgny Bjers added 3 commits August 20, 2020 16:57
- upgrade lodash to avoid vulnerabilities
- upgrade TypeScript to support newer version of GCDLP
- ensure everything still works in Node 8
@tHBp
Copy link

tHBp commented Feb 18, 2022

@geekflyer @JaredBett Any thoughts on this PR please?

@tHBp
Copy link

tHBp commented Feb 18, 2022

Created a new repository and published a new package till the time this PR is merged.

Thanks @tbjers for the PR.

@tbjers
Copy link
Author

tbjers commented Feb 18, 2022

Can't believe this PR has been sitting here for two years now. Thanks for reviving, @tHBp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants