-
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
Updating custom integrations to support multiple identities #5318
Updating custom integrations to support multiple identities #5318
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
identities = list({k for k, v in identity_data.items() if v}) | ||
if len(identities) > 1: | ||
raise FidesopsException( | ||
"Only one identity can be specified for SaaS connector traversal" |
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.
This is the error we're currently seeing, but this isn't accurate since we support multiple identities now
assert len( | ||
access_results[f"{fides_key}:{collection['name']}"] | ||
), f"No rows returned for collection '{collection['name']}'" | ||
if not skip_access_results_check: |
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.
Quick fix to avoid checking for access data. This is necessary since our test account for Responsys isn't going to return access data for every collection.
fides Run #10094
Run Properties:
|
Project |
fides
|
Branch Review |
refs/pull/5318/merge
|
Run status |
Passed #10094
|
Run duration | 00m 42s |
Commit |
866e420c57 ℹ️: Merge 673b1e0bec06c19949b555295e5338de611f9129 into 0a0ea4f400ca6d1e10ad3b154f90...
|
Committer | Adrian Galvan |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
Upgrade your plan to view test results. | |
View all changes introduced in this branch ↗︎ |
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 pretty good. I just have some suggestions on using functions and splitting, but nothing crucial
"Unsupported identity type for Oracle Responsys connector. Currently only `email` and `phone_number` are supported" | ||
) | ||
for identity in get_identities(privacy_request): | ||
if identity == "email": |
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.
We could extract the code fragment for retrieving the query_ids and query_attributes into its own method for readability, now that we are one level deeper with a for loop. It would help readability
…pport-multiple-identities
fides Run #10095
Run Properties:
|
Project |
fides
|
Branch Review |
main
|
Run status |
Passed #10095
|
Run duration | 00m 39s |
Commit |
41094b7274: Updating custom integrations to support multiple identities (#5318)
|
Committer | Adrian Galvan |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
Upgrade your plan to view test results. | |
View all changes introduced in this branch ↗︎ |
Closes PROD-2794
Description Of Changes
Updating the Responsys and Firebase Auth integrations to support multiple identities. These two integrations are custom and were relying on some code that is no longer in use.
Code Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.md