HA-399: migrate existing Cookies resources to Asset resources of type Cookie#5776
HA-399: migrate existing Cookies resources to Asset resources of type Cookie#5776thingscouldbeworse merged 95 commits intomainfrom
Cookies resources to Asset resources of type Cookie#5776Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
adamsachs
left a comment
adamsachs
left a comment
There was a problem hiding this comment.
@thingscouldbeworse looks like you're on the right track here, but there's some nuance/detail that needs to be worked through on the cookies property method 👍
adamsachs
left a comment
adamsachs
left a comment
There was a problem hiding this comment.
this is looking solid @thingscouldbeworse, but two things i think we'll need to address further:
- we need to be careful about our query patterns in the data migration. as it stands now, i'm pretty sure on both upgrade and downgrade we're going to be executing
nqueries forncookie records on Fides. in a worst case scenario, there could be thousands of cookie records on Fides - it's not likely, but there's at least one GVL vendor who declares thousands of cookies (yuck). in any case, it shouldn't be hard for us to gather the data we need "up front" (either via a JOIN in the query, or via a query up front to pull all the privacy declaration data into memory) rather than executenqueries, so i'd encourage us to do that - it'd be good to verify how this interacts with the
fideslangchanges you've got queued up, as mentioned on that PR 👍
There was a problem hiding this comment.
@thingscouldbeworse nice work on the migration updates, that's looking solid now (just wanna confirm you've been able to manually test that since we don't have any automated test coverage [not that we need it] 👍 )
then just a couple of other small cleanup items to work through that i've commented on. i'd like to find some time tomorrow to play around a bit with the API behavior myself if an API client specifies cookies, just to see what happens - but that shouldn't be a blocker.
oh, and looks like you need to bump the migration revision head due to other PRs with migrations!
There was a problem hiding this comment.
OK, i think this is generally looking good! will give a tentative approval since i'm going OOO and don't want to hold this up, though there's at least a couple of things we need to adjust:
- @jpople i think we accidentally removed the
cookiescolumn from the datamap report view, we want to keep that there! can you ensure we add that back in sensibly? probably a simple update but i want to let you do that - updating the fideslang dep 👍
more substantively: i ran through a migration test of existing cookies just now and the resulting asset table looked good from a spot check, but i would also encourage you @thingscouldbeworse and any ohters to run through a few manual tests with migrating locally - it's the aspect of this body of work that is least reversible and hardest to test (repeatedly) on deployed environment, so it carries the most inherent risk.
the other area is the still-unexplained CI adjustments that were needed. i don't think that's a blocker, but if you've got time to dig on it more, i'd do so, and i'd also just keep our eye out for any otherwise unexplained differences in behavior in main that may arise after we do merge this in...
| import os | ||
| from textwrap import dedent | ||
| from typing import Generator | ||
| from typing import Generator, List |
There was a problem hiding this comment.
nit: feels like a stray addition!
| async def test_upsert_system_malformed_privacy_declaration( | ||
| test_config: FidesConfig, system: System, async_session: AsyncSession | ||
| ) -> None: | ||
| with pytest.raises(AttributeError, match="has no attribute 'model_dump'"): | ||
| result = await upsert_system(resources=[system], db=async_session) | ||
|
|
||
|
|
There was a problem hiding this comment.
nit: again this also just feels like a strange addition to have in this PR but i suppose that's OK :)
| columnHelper.accessor((row) => row.cookies, { | ||
| id: COLUMN_IDS.COOKIES, | ||
| cell: (props) => ( | ||
| <GroupCountBadgeCell | ||
| ignoreZero | ||
| suffix="cookies" | ||
| value={props.getValue()} | ||
| {...props} | ||
| /> | ||
| ), | ||
| meta: { | ||
| showHeaderMenu: !isRenaming, | ||
| width: "auto", | ||
| }, | ||
| }), |
thingscouldbeworse
force-pushed
the
HA-399_cookie-to-asset-migration
branch
from
20d275a to
c37669a
Compare
…fides into HA-399_cookie-to-asset-migration
…/ethyca/fides into HA-399_cookie-to-asset-migration
…fides into HA-399_cookie-to-asset-migration
fides
|
||||||||||||||||||||||||||||
| Project |
fides
|
| Branch Review |
main
|
| Run status |
|
| Run duration | 00m 51s |
| Commit |
|
| Committer | Kirk Hardy |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
5
|
| View all changes introduced in this branch ↗︎ | |
4 participants
Closes #HA-399
Description Of Changes
migrate cookies to asset
Code Changes
cookiestable into an entry inassetwithasset_type == 'Cookie'data_usevalue there into the arraydata_usesonassetcookiesproperty onSystemandPrivacyDeclarationand prevent creating cookies when upserting these resourcescookiesproperty which collects assets of typeCookiewith a permissive matching data useSteps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works