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

DynamoDB Connector #2998

Merged
merged 64 commits into from
May 10, 2023
Merged

DynamoDB Connector #2998

merged 64 commits into from
May 10, 2023

Conversation

SteveDMurphy
Copy link
Contributor

@SteveDMurphy SteveDMurphy commented Apr 5, 2023

Closes #2938

Code Changes

  • Add aws connector commands for DynamoDB
  • Include a CLI command to generate a dataset
  • Update the API to generate a DynamoDB dataset
  • Store the primary key and secondary sort (optional) as dataset meta inferred from get_item
  • Add tests for the request_runner_service (access & erasure)
  • Add tests for the CLI command
  • Add tests for the query param generation
  • Create and add an SVG template for the frontend for DynamoDB

Steps to Confirm

  • Set credentials as stored in 1Password
  • Create a connector for Dynamo (the config options are sneakily flipped, not sure how here)
  • Test the connection

Pre-Merge Checklist

Description Of Changes

This is the connector for DynamoDB, the consent component will be worked separately in both documentation and fidesplus.

It appears that some of the other unsafe checks that are not currently passing are being ignored for now, although I have validated the ones added here for DynamoDB appear to be in the clear :)

There have been more than a few migration conflicts so there may be some further need to keep up with more of those changes until this merges

@SteveDMurphy SteveDMurphy self-assigned this Apr 5, 2023
@cypress
Copy link

cypress bot commented Apr 5, 2023

Passing run #1886 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 649f723 into 8dd6881...
Project: fides Commit: 4c5664d8e0 ℹ️
Status: Passed Duration: 00:58 💡
Started: May 10, 2023 4:55 PM Ended: May 10, 2023 4:56 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Patch coverage: 46.96% and project coverage change: -0.45 ⚠️

Comparison is base (c063274) 87.39% compared to head (649f723) 86.94%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2998      +/-   ##
==========================================
- Coverage   87.39%   86.94%   -0.45%     
==========================================
  Files         305      307       +2     
  Lines       18265    18460     +195     
  Branches     2384     2407      +23     
==========================================
+ Hits        15962    16050      +88     
- Misses       1866     1971     +105     
- Partials      437      439       +2     
Impacted Files Coverage Δ
src/fides/api/ops/task/task_resources.py 83.14% <0.00%> (-1.92%) ⬇️
src/fides/api/ctl/routes/generate.py 42.72% <21.42%> (-3.67%) ⬇️
...s/api/ops/service/connectors/dynamodb_connector.py 27.65% <27.65%> (ø)
src/fides/core/dataset.py 78.81% <37.50%> (-3.17%) ⬇️
src/fides/connectors/aws.py 40.44% <38.88%> (-1.22%) ⬇️
src/fides/cli/commands/generate.py 84.33% <76.47%> (-2.03%) ⬇️
...c/fides/api/ops/service/connectors/query_config.py 85.18% <87.50%> (+0.25%) ⬆️
src/fides/api/ops/models/connectionconfig.py 96.03% <100.00%> (+0.03%) ⬆️
...i/ops/schemas/connection_configuration/__init__.py 90.90% <100.00%> (+0.43%) ⬆️
...ction_configuration/connection_secrets_dynamodb.py 100.00% <100.00%> (ø)
... and 1 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

I think this is still in a very rough state after having to rewrite everything this morning to walk back the decision to use pynamodb. Using boto3 natively seemed to make more sense instead of introducing another dependency that didn't do much for us in the end anyways.
@SteveDMurphy SteveDMurphy added the run unsafe ci checks Runs fides-related CI checks that require sensitive credentials label Apr 19, 2023
@SteveDMurphy
Copy link
Contributor Author

I'm still resolving a couple of spooky issues that I think were related to migration conflicts but mostly just planning to add some DynamoDB request specific unit tests and would love any early feedback @galvana !

@SteveDMurphy SteveDMurphy force-pushed the SteveDMurphy-2938-dynamodb-connector branch from c1cc47e to 5eb99c6 Compare May 4, 2023 22:26
@SteveDMurphy
Copy link
Contributor Author

@seanpreston expected checks are good and opened up #3278 for the two that are currently failing

Copy link
Contributor

@seanpreston seanpreston left a comment

Choose a reason for hiding this comment

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

Thanks for those extra changes @SteveDMurphy

@SteveDMurphy SteveDMurphy merged commit ad8fda3 into main May 10, 2023
@SteveDMurphy SteveDMurphy deleted the SteveDMurphy-2938-dynamodb-connector branch May 10, 2023 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run unsafe ci checks Runs fides-related CI checks that require sensitive credentials
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DynamoDB Connector - Consent Policy Spike
3 participants