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

[DANGER] Update to Fideslang 2.0 #3933

Merged
merged 54 commits into from
Sep 13, 2023
Merged

Conversation

ThomasLaPiana
Copy link
Contributor

@ThomasLaPiana ThomasLaPiana commented Aug 17, 2023

Closes #3795

❗ Customer config.json files should be updated as they are migrated to the accompanying release.

Description Of Changes

This PR is focused on bumping the Fideslang version and ensuring that the application is working as expected when set u as a new instance. There is a separate PR for handling migrations for current users, as they are both sufficiently complex pieces to warrant handling separately.

Much of the inspiration/work here is inspired by this PR that handled an earlier, similar change

The Data Migration work is happening here: #4030

WARNING: Merging this change as-is will break any existing instance due to default taxonomy model conflicts

Code Changes

  • bump requirements.txt
  • add a migration for the new version fields
  • Update DataCategories
    • datasets (including collections/fields)
    • policy rule (tested previously, code is copy/pasta)
    • privacy declaration (tested previously, code is copy/pasta)
    • data flows (egress and ingress on System models)
  • Update DataUses
    • privacy declaration (tested previously, code is copy/pasta)
    • policy rule (tested previously, code is copy/pasta)
  • do find/replace for the old categories/uses

Steps to Confirm

Because this PR is focused on getting new instances working instead of migrating data, the Steps to Confirm here are very simple. Tests passing is what we care about here! This PR is where the complex data migration will take place

  • after running nox -s teardown -- volumes and nox -s dev, everything works as intended

Pre-Merge Checklist

@ThomasLaPiana ThomasLaPiana self-assigned this Aug 17, 2023
@cypress
Copy link

cypress bot commented Aug 17, 2023

Passing run #4110 ↗︎

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 4a271f9 into 7f53a75...
Project: fides Commit: 97e809de64 ℹ️
Status: Passed Duration: 01:18 💡
Started: Sep 13, 2023 6:48 PM Ended: Sep 13, 2023 6:49 PM

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

@ThomasLaPiana
Copy link
Contributor Author

ThomasLaPiana commented Aug 17, 2023

@adamsachs update for you before I sign off!

In the description I've linked the fideslang PR where I'm making changes as needed

I'm currently working through a local dev env and shell to run specific tests and try to diagnose why some of the breaks are happening. Focused primarily on the ctl tests as they were easier to run and faster

src/fides/api/db/seed.py Outdated Show resolved Hide resolved
@adamsachs
Copy link
Contributor

@ThomasLaPiana stepping away for a bit now, made a few changes that i think should help get things greener...tried to break it up reasonably by commit so that my changes are easy to follow -- everything is pretty straightforward cleanup so far.

will see if i can circle back a bit later this evening to knock out some more, but leaving this update here in case i don't 👍

@ThomasLaPiana
Copy link
Contributor Author

@adamsachs Thank you! picking it up now

@ThomasLaPiana
Copy link
Contributor Author

there's a final system update test failing here that I suspect @pattisdr might understand, other than that the ctl tests are looking good!

@adamsachs
Copy link
Contributor

adamsachs commented Aug 18, 2023

there's a final system update test failing here that I suspect @pattisdr might understand, other than that the ctl tests are looking good!

🙌 nice @ThomasLaPiana! i think i wrote those system update tests related to privacy declarations so i can take a look, i'll loop in @pattisdr if needed.

@ThomasLaPiana are you working on the ops tests or should i also take a look at that? i've got some bandwidth so i'm happy to, but if you're already on it then i'll let you continue

@ThomasLaPiana
Copy link
Contributor Author

@adamsachs Go for it! I'm signing off for now

@ThomasLaPiana ThomasLaPiana added the run unsafe ci checks Runs fideops-related CI checks that require sensitive credentials label Aug 18, 2023
@ThomasLaPiana
Copy link
Contributor Author

@adamsachs once we get tests passing, we'll merge and ship fideslang 2.0.1 and pin this to a real version

@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

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

Comparison is base (7f53a75) 87.39% compared to head (e7ecbb4) 87.36%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3933      +/-   ##
==========================================
- Coverage   87.39%   87.36%   -0.04%     
==========================================
  Files         320      320              
  Lines       19602    19617      +15     
  Branches     2512     2512              
==========================================
+ Hits        17132    17139       +7     
- Misses       2033     2040       +7     
- Partials      437      438       +1     
Files Changed Coverage Δ
.../api/api/v1/endpoints/consent_request_endpoints.py 85.12% <ø> (ø)
.../api/api/v1/endpoints/privacy_request_endpoints.py 90.34% <ø> (ø)
src/fides/api/db/seed.py 89.20% <ø> (ø)
src/fides/api/models/privacy_request.py 96.30% <ø> (ø)
src/fides/api/schemas/privacy_request.py 100.00% <ø> (ø)
.../api/service/connectors/consent_email_connector.py 97.14% <ø> (ø)
...vice/saas_request/saas_request_override_factory.py 100.00% <ø> (ø)
src/fides/api/util/data_category.py 100.00% <ø> (ø)
.../fides/api/api/v1/endpoints/messaging_endpoints.py 92.94% <100.00%> (+0.12%) ⬆️
src/fides/api/models/sql_models.py 98.12% <100.00%> (+0.07%) ⬆️

... and 2 files with indirect coverage changes

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

@adamsachs
Copy link
Contributor

adamsachs commented Aug 18, 2023

OK! i think we've got all non-external tests passing now!

@pattisdr would you mind taking a quick peak at the ops tests i fixed in 8ec6a87, since some of them are touching consent-related tests and i want to be extra sure i'm not misunderstanding the tests and invalidating them 🙏

if that looks good, i think we've got our non-external/unsafe tests good to go! the remaining items to get this ready to merge would be:

  • look into external test failures to determine whether these are expected/in main already or caused by some code changes here
  • holistic review of this PR (i've focused on getting CI green up to this point)
  • cut fideslang 2.0.1 per @ThomasLaPiana's note above, and update our dependency here

i should be able to make some progress on the first two items shortly...

@ThomasLaPiana
Copy link
Contributor Author

spoke a bit with @pattisdr and one theory that could explain this exceptiongroup.ExceptionGroup: unhandled errors in a TaskGroup type of error from cropping up in all sorts of (seemingly unrelated) places all of a sudden is that some recent change (likely in a dependency of ours?) is causing underlying errors to get swallowed and instead be thrown up with this generic exception type.

@ThomasLaPiana do you have any further leads? i haven't dug into this much, just wanted to put this theory down on paper here. i think @pattisdr is also continuing further investigation on her end 👍

I moved this to a new issue: #4020

@ThomasLaPiana ThomasLaPiana changed the title Update Fideslang to version 2 Update to Fideslang 2.0 Sep 5, 2023
@ThomasLaPiana
Copy link
Contributor Author

I'm moving the migration-specific work to a new PR, to de-clutter this PR and focus it specifically on getting fides running with 2.0

@ThomasLaPiana ThomasLaPiana mentioned this pull request Sep 6, 2023
22 tasks
@ThomasLaPiana ThomasLaPiana added the high-risk This issue suggests changes that have a high-probability of breaking existing code label Sep 6, 2023
@ThomasLaPiana ThomasLaPiana changed the title Update to Fideslang 2.0 [DANGER] Update to Fideslang 2.0 Sep 6, 2023
@pattisdr
Copy link
Contributor

pattisdr commented Sep 12, 2023

I think we need to update the config.json files we ship with, PR here #4076 although I'm going to need some FE assistance

EDIT: done, thank you Allison!

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.

Did another review here, although I didn't go through every single open comment.

Giving this a tentative 👍 to not hold up the overnight crew, assuming the corresponding migration will be merged into this branch first, this branch will be updated with main, comments will be resolved, and that tests are passing.

❗ Note that main has had a new migration merged in, you'll need to bump the downrev of this migration to match that one.

Separately, I think we need the config.json files we ship with to be updated, but FE help is needed #4076. We could also merge this in separately tomorrow.

Co-authored-by: Allison King <allisonjuliaking@gmail.com>
@pattisdr
Copy link
Contributor

OK @ThomasLaPiana, Allison helped me with the FE, so these config.json changes are merged here -

@ThomasLaPiana
Copy link
Contributor Author

it looks like fideslang 2.0 was missing a change that got patched into 1.4.6, I cut another release of Fideslang (2.0.3) that should contain the change and get things working here

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.

🎊 let's do this!

@adamsachs adamsachs merged commit e8f656f into main Sep 13, 2023
42 of 49 checks passed
@adamsachs adamsachs deleted the ThomasLaPiana-update-fideslang-200 branch September 13, 2023 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high-risk This issue suggests changes that have a high-probability of breaking existing code run unsafe ci checks Runs fideops-related CI checks that require sensitive credentials
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fideslang taxonomy migrations for version 2.0.0
4 participants