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

3037 support privacy declaration resource type for custom fields #3149

Merged
merged 3 commits into from
Apr 25, 2023

Conversation

adamsachs
Copy link
Contributor

@adamsachs adamsachs commented Apr 25, 2023

❗ Contains migration; check downrev before merge

partially closes #3037

there will need to be some follow up changes on the fidesplus API side to make this fully work, but these are the necessary updates in fides

Code Changes

  • update sqlalchemy and DB resource_type enum for CustomFieldDefinition and CustomField tables/models to allow privacy_declaration as a value
    • include a migration for this change. migration downgrade path has to remove any records (CustomFields and CustomFieldDefinitions) that have the privacy_declaration as their resource_type

Steps to Confirm

  • The only real testing we can do on this PR specifically would be around the migrations.
    • I tested locally that migrating from a previous app state successfully added the privacy_declaration value to the resourcetypes enum
  • Testing the actual application functionality will require some updates to the API, which lives in fidesplus

Pre-Merge Checklist

@adamsachs adamsachs changed the title 3037 support privacy declaration resource type for custom fields #2 3037 support privacy declaration resource type for custom fields Apr 25, 2023
@cypress
Copy link

cypress bot commented Apr 25, 2023

Passing run #1562 ↗︎

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

Details:

Merge 6f2e02b into 86f4bd8...
Project: fides Commit: 9d08a30add ℹ️
Status: Passed Duration: 00:36 💡
Started: Apr 25, 2023 3:19 PM Ended: Apr 25, 2023 3:19 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 25, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.04 ⚠️

Comparison is base (61cbd96) 87.51% compared to head (f51bd23) 87.48%.

❗ Current head f51bd23 differs from pull request most recent head 6f2e02b. Consider uploading reports for the commit 6f2e02b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3149      +/-   ##
==========================================
- Coverage   87.51%   87.48%   -0.04%     
==========================================
  Files         309      309              
  Lines       17993    17925      -68     
  Branches     2337     2325      -12     
==========================================
- Hits        15746    15681      -65     
+ Misses       1821     1820       -1     
+ Partials      426      424       -2     
Impacted Files Coverage Δ
src/fides/api/ctl/sql_models.py 98.26% <100.00%> (+<0.01%) ⬆️

... and 8 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

CHANGELOG.md Show resolved Hide resolved
@adamsachs adamsachs marked this pull request as ready for review April 25, 2023 02:08
@adamsachs
Copy link
Contributor Author

@SteveDMurphy since you've touched some custom fields work, thought you may be good to review this?
also @TheAndrewJackson you're working on FE, not sure how familiar you are with the custom fields BE, but you can take a look too!

a real small change in fides, some corresponding changes in fidesplus coming shortly thereafter

@adamsachs adamsachs force-pushed the 3037-support-privacy-declaration-resource-type_2 branch from f51bd23 to c6d5a9f Compare April 25, 2023 14:43
Copy link
Contributor

@SteveDMurphy SteveDMurphy left a comment

Choose a reason for hiding this comment

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

These ones are tough to test for sure! Also feels like there have been 1-2 migration changes per day making it hard to keep them aligned and not blocking each other in flight 🥴 looks good for me locally and runs as well

Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

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

looks good @adamsachs

@adamsachs adamsachs merged commit c350a5e into main Apr 25, 2023
@adamsachs adamsachs deleted the 3037-support-privacy-declaration-resource-type_2 branch April 25, 2023 15:40
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.

Update custom fields to support new "privacy declaration" resource type once DB class is created
3 participants