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

Allow for anonymization of in-memory configurations #186

Merged
merged 5 commits into from
May 23, 2023

Conversation

Kircheneer
Copy link
Contributor

@Kircheneer Kircheneer commented May 21, 2023

  • Introduce build_anonymizers function to centralize configuration logic
  • Introduce anonymize_configuration function to anonymize configurations in memory
  • Reduce newly created code duplication by using anonymize_configuration to anonymize configuration in files
  • Write a test for this

This change is Reviewable

- Introduce build_anonymizers function to centralize configuration logic
- Introduce anonymize_configuration function to anonymize configurations
  in memory
- Reduce newly created code duplication by using anonymize_configuration
  to anonymize configuration in files
@Kircheneer
Copy link
Contributor Author

Why? I am building something for which I want to anonymize configurations before I even have them on the disk, as I never want them on the disk un-anonymized.

Copy link
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @Kircheneer and @sfraint)

a discussion (no related file):
This looks cool, thanks for the contribution. A few notes:

  1. We didn't quite have our new GitHub-based CI setup to run on PRs, fixed now. I merged the updated config in, and it found a bunch of style issues. Most were fixed automatically (pre-commit run --all-files), please take a pass.

  2. I'd recommend a simpler / less invasive approach that doesn't require quite so much boilerplate. TL;DR:

    • introduce a (private-ish) class that is a SingleFileAnonymizer. That class will be the single source of truth for all these settings, and also only do the logic ones.
    • give it an anonymize_io(in_io, out_io) function that does the work
    • make the existing function create the class once and then recursively call anonymize_io using file IO objects
    • make your new function create the class once and call it with StringIO (or BytesIO, I forget which)

Seems like this approach would be simpler because we'd eliminate the need for this new kwargs dictionary as well as provide reusability.


@Kircheneer
Copy link
Contributor Author

Thanks for the quick review! I believe I have addressed all your concerns and the CI should pass - I ran pre-commit run --all-files locally and there were no issues.

Let me know if this matched your desired design now.

@Kircheneer Kircheneer force-pushed the lk-in-memory-operations branch from a627094 to 72824f8 Compare May 23, 2023 19:09
Copy link
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Kircheneer and @sfraint)


tests/unit/test_anonymize_files.py line 9 at r3 (raw file):

from netconan.anonymize_files import FileAnonymizer, anonymize_files

Do you know how this got added? Wasn't triggered by pre-commit run --all-files for me. Please revert if so.

@Kircheneer
Copy link
Contributor Author

Reviewed 2 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Kircheneer and @sfraint)

tests/unit/test_anonymize_files.py line 9 at r3 (raw file):

from netconan.anonymize_files import FileAnonymizer, anonymize_files

Do you know how this got added? Wasn't triggered by pre-commit run --all-files for me. Please revert if so.

Fixed!

Copy link
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Kircheneer and @sfraint)

@Kircheneer Kircheneer force-pushed the lk-in-memory-operations branch from db7d08e to f616bc1 Compare May 23, 2023 19:39
@Kircheneer
Copy link
Contributor Author

Fixed CI again now, sorry.

Copy link
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Kircheneer and @sfraint)

@dhalperi dhalperi merged commit 5869840 into intentionet:master May 23, 2023
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.

2 participants