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

Improve DynamoDB Field Generation and Error Handling #4943

Merged
merged 10 commits into from
Jun 4, 2024

Conversation

SteveDMurphy
Copy link
Contributor

@SteveDMurphy SteveDMurphy commented Jun 3, 2024

Closes FIDES-635

Description Of Changes

  • Scans Dynamo (ignoring values) to generate a more accurate field description
  • Handles errors generating from incomplete permissions (either to view or operate on resources)
  • Optionally Generate DynamoDB datasets individually instead of nested into a single Dataset

Code Changes

  • Added CLI option to have separate datasets
  • Handle exceptions from Boto3 around permissions errors
  • Scan the DynamoDB table for Fields
  • Ensure pagination is handled correctly

Steps to Confirm

  • credentials_id shared in 1Password to use for testing with

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation:
    • documentation complete, PR opened in fidesdocs
    • documentation issue created in fidesdocs
    • if there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md
  • For API changes, the Postman collection has been updated
  • If there are any database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!

@SteveDMurphy SteveDMurphy self-assigned this Jun 3, 2024
Copy link

vercel bot commented Jun 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Visit Preview Jun 4, 2024 6:40pm

@SteveDMurphy SteveDMurphy force-pushed the SteveDMurphy-fides-635-improve-dynamo-generate branch from 01c672e to 1ced7fb Compare June 3, 2024 16:37
Copy link

cypress bot commented Jun 3, 2024

Passing run #8106 ↗︎

0 4 0 0 Flakiness 0

Details:

Merge c769649 into 1b0acd8...
Project: fides Commit: 850cb60000 ℹ️
Status: Passed Duration: 00:34 💡
Started: Jun 4, 2024 6:51 PM Ended: Jun 4, 2024 6:51 PM

Review all test suite changes for PR #4943 ↗︎

Copy link

codecov bot commented Jun 3, 2024

Codecov Report

Attention: Patch coverage is 17.07317% with 34 lines in your changes missing coverage. Please review.

Project coverage is 86.86%. Comparing base (d50f5b3) to head (c769649).
Report is 2 commits behind head on main.

Files Patch % Lines
src/fides/connectors/aws.py 13.88% 31 Missing ⚠️
src/fides/api/api/v1/endpoints/generate.py 0.00% 1 Missing ⚠️
src/fides/cli/commands/generate.py 50.00% 1 Missing ⚠️
src/fides/core/dataset.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4943      +/-   ##
==========================================
- Coverage   86.92%   86.86%   -0.06%     
==========================================
  Files         346      346              
  Lines       21209    21253      +44     
  Branches     2785     2793       +8     
==========================================
+ Hits        18435    18461      +26     
- Misses       2285     2304      +19     
+ Partials      489      488       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SteveDMurphy SteveDMurphy requested a review from adamsachs June 4, 2024 10:57
@SteveDMurphy SteveDMurphy marked this pull request as ready for review June 4, 2024 10:58
Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

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

@SteveDMurphy nice job cleaning this up! i made one minor change on my end that allows us to fully utilize these underlying functions in the new monitor code over in plus, hope that's alrigh.t

generally all these changes seem sound to me, and i've tested them with an eye to how they integrate with new dynamo monitor code i'm working on in plus (will put the PR link once i've got the PR up).

i haven't tested or looked super closely into the changes in relation to our CLI workflows - but i trust that you've done due diligence there (and i know you've been testing it!). coupled with the fact that CI is passing, i feel comfortable merging this 👍

@adamsachs adamsachs merged commit d21eef5 into main Jun 4, 2024
41 of 42 checks passed
@adamsachs adamsachs deleted the SteveDMurphy-fides-635-improve-dynamo-generate branch June 4, 2024 19:18
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