-
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
fideslang 3.0 upgrades (language changes only, no pydantic updates!) #4502
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
a20bbc5
to
b12a17e
Compare
Passing run #5675 ↗︎
Details:
Review all test suite changes for PR #4502 ↗︎ |
cbbe934
to
121363b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4502 +/- ##
==========================================
- Coverage 87.09% 87.06% -0.04%
==========================================
Files 332 333 +1
Lines 20514 20465 -49
Branches 2642 2641 -1
==========================================
- Hits 17867 17817 -50
- Misses 2180 2181 +1
Partials 467 467 ☔ View full report in Codecov by Sentry. |
6b09022
to
45e8992
Compare
21d8dbf
to
0458212
Compare
73aa070
to
3a8b121
Compare
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.
looking great Adam.
The main thing I'd want to check - do API requests ignore when deprecated values are supplied (which is what I'd expect) or do we get failures? I think "ignore" is Pydantic's default but I'm not sure every schema has that set.
- name: legal_basis | ||
data_categories: | ||
- system.operations | ||
data_qualifier: aggregated.anonymized.unlinked_pseudonymized.pseudonymized.identified | ||
- name: legitimate_interest | ||
description: Boolean value denoting whether or not the data use is marked as | ||
a legitimate interest | ||
data_categories: | ||
- system.operations | ||
data_qualifier: aggregated.anonymized.unlinked_pseudonymized.pseudonymized.identified | ||
- name: legitimate_interest_impact_assessment |
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.
Nice, did you remember to go into this file and remove this or was it automatically flagged somewhere? I've felt this file only gets flagged if it's missing values not if there are too many
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.
i think you're right on that behavior. pretty sure i found the references just via a global string search on the codebase 🤷
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.
so, definitely error prone, if i've missed something!
@@ -1434,12 +1434,6 @@ def test_update_system_manager_existing_system_not_in_request_which_removes_syst | |||
"description": "fixture-made-system", | |||
"organization_fides_key": "default_organization", | |||
"system_type": "Service", | |||
"data_responsibility_title": "Processor", |
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 also remove this whole test file: tests/ops/migration_tests/test_system_dictionary_data_migration.py
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.
ah i did consider removing that but i felt that the migration itself will still technically run, so in some senses, it's still valid to have those tests in place?? anyway, didn't see a whole lot of harm in keeping it in place because it was all passing without needing any updates 🤷
op.drop_column("ctl_datasets", "data_qualifier") | ||
op.drop_index("ix_ctl_systems_name", table_name="ctl_systems") | ||
op.drop_column("privacydeclaration", "data_qualifier") | ||
op.drop_constraint("purpose_constraint", "tcf_purpose_overrides", type_="unique") |
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.
ah this is my fault not properly defined on model, fixed in overhaul PR
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.
ah right i hadn't even noticed that here, shows you how closely i inspected the autogenerated migrations 😬
i'll remove from here, thanks for catching!
) | ||
op.create_table( | ||
"ctl_data_qualifiers", | ||
sa.Column("id", sa.VARCHAR(length=255), autoincrement=False, nullable=False), |
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.
in the downrevs, I'd update all these to be nullable=True so it can actually be downgraded
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.
thank you, great catch!
yup, it looks like it! here's some manual testing i've done, let me know if this looks to cover things decently:
|
3a8b121
to
4d20bc5
Compare
ah such thorough testing thank you 🙏 #4502 (comment) Do you anticipate any issues with previously-saved data with deprecated fields being serialized? I don't think it's a problem similar logic to your endpoints (thinking of some of the fields where data is stored in json) |
4d20bc5
to
1e1005d
Compare
hmm, well shouldn't all that data be removed as part of the migrations? i may be missing something or misunderstanding what you mean! |
I was thinking about fields stored as json? like |
Ah right, forgot about the embedded JSON fields - i'll look to confirm that tomorrow 👍 |
0a247bf
to
0045c35
Compare
0045c35
to
55c5ca1
Compare
OK, this is looking good. just to document what i did:
so i think we're looking good on this front! |
excellent thank you for verifying @adamsachs 🏆 |
55c5ca1
to
49892c7
Compare
as mentioned @pattisdr , i think this is just about ready for a final review! i've done some amount of manual testing here and in fidesplus, but i'll look to be a bit more thorough in the next day or two 👍 |
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.
nice careful work here 👍
Closes PROD-1490
fideslang
3.0Description Of Changes
Removes references to constructs that are being removed in
fideslang
3.0.0.Removals:
DataQualifier
Registry
System
fields (already had been deprecated):joint_controller
third_country_transfers
data_responsibility_title
data_protection_impact_assessment
Dataset
fields (already had been deprecated):joint_controller
data_qualifier
retention
third_country_transfers
DataUse
fields (already had been deprecated):legal_basis
special_category
recipients
legitimate_interest
legitimate_interest_impact_assessment
Code Changes
Steps to Confirm
fidesplus
too: PR here https://github.com/ethyca/fidesplus/pull/1267Pre-Merge Checklist
CHANGELOG.md