-
Notifications
You must be signed in to change notification settings - Fork 72
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
Fides con 122 simon data erasure connector #4552
Fides con 122 simon data erasure connector #4552
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Passing run #6615 ↗︎
Details:
Review all test suite changes for PR #4552 ↗︎ |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4552 +/- ##
=======================================
Coverage 86.67% 86.67%
=======================================
Files 336 336
Lines 20089 20089
Branches 2581 2581
=======================================
Hits 17413 17413
Misses 2203 2203
Partials 473 473 ☔ View full report in Codecov by Sentry. |
Still a couple of things to complete here but I wanted this available for review as I don't think they are going to impact the overall operation, e.g. I am working on getting the docs up and out, I still need to change the changelog as well. |
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.
Good work with this one, the structure looks good and the tests all passed! I just left a few comments.
Co-authored-by: Adrian Galvan <[email protected]>
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.
Just the changelog update and a question around the request body @MarcGEthyca - nice work here!
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.
Looks good @MarcGEthyca ! Thanks for following through on this, added one update suggestion for the changelog is all
Co-authored-by: Steve Murphy <[email protected]>
Thanks to both @SteveDMurphy and @galvana for their reviews! I think we're all set except for the Vault setup and configuring. My local tests run without issue but while I can submit a request with Simon Data integration, I can't see the request to approve it at this point. This was just a misunderstanding/misremembering on my part. Adrian sorted me out =) |
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.
Thanks for the changes, looks good to me!
Closes #CON-122
Description Of Changes
Added configuration, dataset, fixtures, and tests for Simon Data Erasure only connector
Code Changes
Pre-PR checklist
connector_params
identity_email
oridentity_phone_number
@galvana we talked about having me start doing this part just before ... the thing. I'm not sure we finished off whatever we had to do there. I'm down to walkthrough it whenever!
https://github.com/ethyca/fidesdocs/pull/315
https://github.com/ethyca/fidesdocs/issues/316
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.md